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

don't re-indent function declarations with scope < "indention" #304

Merged
merged 4 commits into from Jan 3, 2018

Conversation

lorenzwalthert
Copy link
Collaborator

Closes #303

@codecov-io
Copy link

codecov-io commented Dec 26, 2017

Codecov Report

Merging #304 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   91.18%   91.17%   -0.02%     
==========================================
  Files          30       30              
  Lines        1373     1371       -2     
==========================================
- Hits         1252     1250       -2     
  Misses        121      121
Impacted Files Coverage Δ
R/style_guides.R 96.66% <100%> (-0.06%) ⬇️

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 a7fff7a...0780c41. Read the comment docs.

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks! Makes sense to me.

R/style_guides.R Outdated
update_indention_ref_fun_dec,
NULL
if (scope >= "indention") update_indention_ref_fun_dec,
identity
Copy link
Member

Choose a reason for hiding this comment

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

I'm not in love with the use of identity here, this sure doesn't help performance. Maybe allow NULL and filter out later with compact()? Need to use lst() instead of c().

The name for the list elements will be the full quoted expression, maybe change l. 138 above to wrap_if_else_multi_line_in_curly = if (strict) wrap_if_else_multi_line_in_curly else identity and likewise in other places?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I like the idea with purrr::compact(), lst() and naming the arguments here and in other places.

and filter with purrr:::compact(). Requires breaking API change since initializer are now not a function anymore but a list of funcitons.
API Outdated
@@ -2,7 +2,7 @@

## Exported functions

create_style_guide(initialize = default_style_guide_attributes, line_break = NULL, space = NULL, token = NULL, indention = NULL, use_raw_indention = FALSE, reindention = tidyverse_reindention())
create_style_guide(initialize = lst(default_style_guide_attributes), line_break = NULL, space = NULL, token = NULL, indention = NULL, use_raw_indention = FALSE, reindention = tidyverse_reindention())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we use lst() as a default in an exported function at all?

@lorenzwalthert
Copy link
Collaborator Author

lorenzwalthert commented Jan 3, 2018

Unfortunately, there was an inconsistency in create_style_guide(), which is that all inputs are list of transformers, except initialize, which is just a function (and not a list of functions). Should we make a breaking API change here so we can map conveniently and have a more consistent API or not? I suggest we internally convert to a list and keep the API as is.

and convert initialize internally to a list of one function.
@krlmlr
Copy link
Member

krlmlr commented Jan 3, 2018

It looks more natural to accept a function for the initialize argument, but there may be use cases where a list of functions is helpful. Agree to keep the API and use lst() internally (and not in the default argument).

@lorenzwalthert
Copy link
Collaborator Author

Ok, agree.

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.

None yet

3 participants