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

Avoid spaces around ^ #308

Merged
merged 4 commits into from Feb 23, 2018
Merged

Avoid spaces around ^ #308

merged 4 commits into from Feb 23, 2018

Conversation

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Dec 30, 2017

following tidyverse/style#46.

@krlmlr krlmlr requested a review from lorenzwalthert Dec 30, 2017
@codecov-io
Copy link

@codecov-io codecov-io commented Dec 30, 2017

Codecov Report

Merging #308 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   91.35%   91.36%   +0.01%     
==========================================
  Files          30       30              
  Lines        1399     1402       +3     
==========================================
+ Hits         1278     1281       +3     
  Misses        121      121
Impacted Files Coverage Δ
R/style_guides.R 96.8% <100%> (+0.07%) ⬆️
R/rules-spacing.R 96.07% <100%> (ø) ⬆️

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 49621d9...0c7e298. Read the comment docs.

@lorenzwalthert
Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Dec 31, 2017

Aren't we changing the defaults in specify_math_token_spacing()? Because currently, all the defaults correspond to the tidyverse style guide. Or do you consider that API change as too disruptive?

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Dec 31, 2017

I meant to change the default, the tidyverse style guide now contains the rules for ^. What am I missing?

@lorenzwalthert
Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Dec 31, 2017

I thought we change the default value in the function declaration of specify_math_token_spacing() (I.e. here: https://github.com/r-lib/styler/pull/308/files#diff-fefe826487d2bf7becb27b255b8bd8dfR304) , not in the function call.

@@ -1 +1 @@
1 + +1 - 3 / 23 * 3 ^ 4
1 + +1 - 3 / 23 * 3 ^4
Copy link
Member Author

@krlmlr krlmlr Jan 1, 2018

I have no idea why this happens:

styler::style_text("1 - 3 * 4 ^ 5", strict = FALSE)
#> 1 - 3 * 4 ^ 5

Copy link
Collaborator

@lorenzwalthert lorenzwalthert Jan 1, 2018

That's actually what I'd expect because strict = FALSE means no strict implementation, right? Or do you prefer to set the spaces to exactly zero around ^ even if strict = FALSE?

Copy link
Collaborator

@lorenzwalthert lorenzwalthert Jan 1, 2018

I think the reason this might seem inconsistent with the other test is that there is no space around ^ in any of the other *-out.R files, but I think that is because in all *-in.R files, we always have x^k(so no space in between) and hence the tests don't actually test whether x ^ k is turned into x^k.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert left a comment

Thanks. I think as mentioned in the comment above, do you think strict = FALSE should remove spaces around ^? If yes, we need to adapt.

@krlmlr krlmlr force-pushed the f-no-spacing-caret branch from 9e3effa to 0c7e298 Feb 23, 2018
@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Feb 23, 2018

I've updated the code, because we're removing spaces for : too:

styler::style_text("1 : 10", strict = FALSE)
#> 1:10
styler::style_text("1 ^ 10", strict = FALSE)
#> 1^10

Created on 2018-02-23 by the reprex package (v0.2.0).

@lorenzwalthert
Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Feb 23, 2018

Great. It payed off to reduce some duplication and encapsulate common functionality in style_space_around_token().

@lorenzwalthert lorenzwalthert merged commit 15689bb into master Feb 23, 2018
6 checks passed
@krlmlr krlmlr deleted the f-no-spacing-caret branch Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants