Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding styler support #1581

Merged
merged 8 commits into from
Aug 10, 2018
Merged

Adding styler support #1581

merged 8 commits into from
Aug 10, 2018

Conversation

lorenzwalthert
Copy link
Contributor

Finally, I got time to try to add styler support as suggested in #1516. I have the following questions:

  • As of now, there are two options tidy (defaults to FALSE) and tidy.method (defaults to "formatR"). We could remove the tidy option. This would result in a backward-incompatible API change, which is probably not a good idea. I think it would make sense to simply issue a warning when someone sets tidy.method = "styler" and (either because of explicit specification or because of the default) tidy = FALSE, because he or she probably just wants tidy = TRUE and tidy.method = styler. I have not yet implemented that, so let me know if I should go ahead with that.
  • I don't know what resources (e.g. documentation, cheat sheets etc.) need to be updated to reflect the API change, at least I could not find any help file where the tidy option was documented.
  • Also, I wasn't sure about if and how I should add unit tests for this.

Could you please advise on these points? Thanks.

cc: @krlmlr

@krlmlr
Copy link
Contributor

krlmlr commented Jul 29, 2018

How about supporting tidy = "styler" and tidy = "formatR", in addition to TRUE and FALSE ?

Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krlmlr Yes, I like the idea of using one option tidy = 'formatR' or 'styler'. To make it backward-compatible, tidy = TRUE means tidy = 'formatR'. We could even make this option accept a function, so that it is completely extensible.

@lorenzwalthert I'll just tweak the PR by myself and merge it. Thanks!

@yihui yihui added this to the v1.21 milestone Aug 10, 2018
@yihui yihui merged commit 01407a7 into yihui:master Aug 10, 2018
@lorenzwalthert lorenzwalthert deleted the styler branch August 10, 2018 21:53
@lorenzwalthert
Copy link
Contributor Author

Cool, thanks @yihui.

yihui added a commit to yihui/yihui.org that referenced this pull request Aug 13, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants