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

Use | more judiciously #2028

Merged
merged 8 commits into from
Aug 2, 2023
Merged

Use | more judiciously #2028

merged 8 commits into from
Aug 2, 2023

Conversation

MichaelChirico
Copy link
Collaborator

@MichaelChirico MichaelChirico commented Aug 1, 2023

Re-examined in what situations | can be used.

Till now we've often conservatively used //NODE1[ $BIG_EXPRESSION ] | //NODE2[ $BIG_EXPRESSION ], but this works fine: (//NODE1 | //NODE2)[ $BIG_EXPRESSION ].

Untested as yet but I am assuming this can have some efficiency benefit (or at least not hurt, while hopefully making the code more readable).

Marking as draft for now since infix_spaces_linter() will likely benefit and #2025 is pending, but feel free to review the existing changes. I'll also try and add some timings.

@MichaelChirico MichaelChirico marked this pull request as draft August 1, 2023 08:06
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #2028 (e3819ce) into main (f9bf506) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head e3819ce differs from pull request most recent head 5463bf1. Consider uploading reports for the commit 5463bf1 to get more accurate results

@@           Coverage Diff           @@
##             main    #2028   +/-   ##
=======================================
  Coverage   98.55%   98.55%           
=======================================
  Files         113      113           
  Lines        4994     4995    +1     
=======================================
+ Hits         4922     4923    +1     
  Misses         72       72           
Files Changed Coverage Δ
R/function_argument_linter.R 100.00% <100.00%> (ø)
R/indentation_linter.R 100.00% <100.00%> (ø)
R/infix_spaces_linter.R 100.00% <100.00%> (ø)
R/pipe_continuation_linter.R 100.00% <100.00%> (ø)
R/redundant_equals_linter.R 100.00% <100.00%> (ø)
R/vector_logic_linter.R 100.00% <100.00%> (ø)

R/indentation_linter.R Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Collaborator Author

Very little (if any) improvement on redundant_equals_linter(), so the readability improvement will be what pushes this change. If we think readability is harmed I can revert.

exp <- get_source_expressions("QC.R")
l <- redundant_equals_linter()

# AT HEAD
system.time(replicate(500, lapply(exp$expressions, l)))
#    user  system elapsed 
#  35.225   0.172  35.577 

# AT ae0551b
system.time(replicate(500, lapply(exp$expressions, l)))
#    user  system elapsed 
#  35.147   0.120  35.392 

Checking on indentation_linter() now but there's so much other overhead there it's hard to compare. I'll check infix_spaces_linter() after #2025 is merged.

R/indentation_linter.R Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Collaborator Author

Oh interesting, infix_spaces_linter() does wind up faster... 50% so!

# HEAD
system.time(replicate(500, lapply(exp$expressions, l)))
#    user  system elapsed 
# 171.315   0.280 172.313 

# d4e93e1
system.time(replicate(500, lapply(exp$expressions, l)))
#    user  system elapsed 
#  83.125   0.118  83.554

@MichaelChirico MichaelChirico marked this pull request as ready for review August 2, 2023 07:09
R/infix_spaces_linter.R Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Collaborator Author

Lint will go away after merging #2033

@MichaelChirico MichaelChirico merged commit 2b62a3a into main Aug 2, 2023
21 checks passed
@MichaelChirico MichaelChirico deleted the node-pipe branch August 2, 2023 20:07
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