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

Allow selecting the style used by RStudio addins. #463

Merged
merged 11 commits into from
Feb 16, 2019

Conversation

riccardoporreca
Copy link
Contributor

  • Option "styler.addins.style" stores the selected style.
  • New addin to set the style.
  • Existing addins adapted to use the selected style.

* "styler.addins.style" option stores the selected style.
* New addin to set the style.
* Existing addins adapted to use the selected style.
@codecov-io
Copy link

codecov-io commented Jan 23, 2019

Codecov Report

Merging #463 into master will decrease coverage by 1.95%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #463      +/-   ##
=========================================
- Coverage   90.65%   88.7%   -1.96%     
=========================================
  Files          36      36              
  Lines        1627    1664      +37     
=========================================
+ Hits         1475    1476       +1     
- Misses        152     188      +36
Impacted Files Coverage Δ
R/addins.R 0% <0%> (ø) ⬆️
R/utils.R 71.87% <0%> (+3.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b7ea1b...de626c9. Read the comment docs.

R/addins.R Outdated

# Dedicated binding for package styling addin. Simple wrapper calling style_pkg
# with the selected addins style.
style_package <- function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't want another Addin, see also #250.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually thanks go @jonmcalder we have figured out that you have not registered a new Addin, just moved existing code. I overlooked that. Sorry. Will add in #500 again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this was intentional, as discussed when integrating your tweaks in miraisolutions#1 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, the confusion arose because people wanted to add other Addins like #476 and #250 and in the diff, it looked as if you added an Addin for package styling. This was not the case, you just moved some code. So that was my mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, no problem!

@lorenzwalthert
Copy link
Collaborator

First of all, thanks @riccardoporreca for the PR. I think I need to test it myself and then I will review the PR.

@riccardoporreca
Copy link
Contributor Author

@lorenzwalthert, sure, I will also be happy to discuss if, despite the reasonable point of not cluttering the addins, the concept and utilities for setting and using a given style guide for the addins could be still useful.

@lorenzwalthert
Copy link
Collaborator

Thanks. Just to make it clear, I see that the PR has two parts and I also think the functionality about style selection is something we'd want to support. Apart from the implementation details, one thing I am not sure about is how to deal with global variables and environment variables and default values. I created fallback and integrated it into styler (compare #319), but I am not sure if it's the best solution. I.e. if the default style should be set in a yaml file.

@lorenzwalthert
Copy link
Collaborator

I changed my mind and we will defer the questions regarding default values and just go with R options for now. Hope to find time to review soon.

lorenzwalthert and others added 4 commits February 13, 2019 21:17
Co-Authored-By: lorenzwalthert <lorenz.walthert@icloud.com>
Co-Authored-By: lorenzwalthert <lorenz.walthert@icloud.com>
@lorenzwalthert lorenzwalthert merged commit 893c975 into r-lib:master Feb 16, 2019
@lorenzwalthert
Copy link
Collaborator

thanks @riccardoporreca. There is one minor thing about error messages which I will fix when working on #472. The Add-in has passed my manual test with the oneline style guide too, so it's all fine 😄.

@riccardoporreca
Copy link
Contributor Author

Great, well spotted with the error message and the concise rlang alternative.

Thanks @lorenzwalthert for the nice collaboration on the PR.

@lorenzwalthert
Copy link
Collaborator

@riccardoporreca I just realized that instead of asking for a style, we should maybe ask for transformers, because then, people could also specify arguments thereof, e.g. the scope:

style_text(transformers = tidyerse_style(scope = "spaces", ...)

This is not currently possible because the input for the Addin is a style, i.e.

styler::tidyverse_style

and not transformers, e.g.

styler::tidyverse_style()

@riccardoporreca
Copy link
Contributor Author

@riccardoporreca I just realized that instead of asking for a style, we should maybe ask for transformers, because then, people could also specify arguments thereof

@lorenzwalthert, I agree.

I am not sure how you would describe the addin with this new approach, "transformers" is somehow a less intuitive term than "style" for the user, so maybe the addin can still be called "Set Style" and described as "Prompt for and set the style transformers used by all STYLER addins", or something more sensible. Similar for the prompt and error messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants