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.

Sign up for GitHub

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

Jump to bottom

promote Performance/LstripRstrip to Style/LstripRstrip #6637

Closed
matkoniecz opened this issue Jan 8, 2019 · 2 comments
Closed

promote Performance/LstripRstrip to Style/LstripRstrip #6637

matkoniecz opened this issue Jan 8, 2019 · 2 comments
Labels
enhancement good first issue Easy task, suitable for newcomers to the project

Comments

@matkoniecz
Copy link

matkoniecz commented Jan 8, 2019 •
edited

Unlike many other Performance cops Performance/LstripRstrip is not making code less readable and is a good idea independent from performance improvements.

As #5977 will cause performance cops to be disabled by default I suggest to rescue this one as it is useful to avoid lstrip.rstrip where strip that is more readable and shorter exists.

It may be also useful to expand description with a note that this change may improve performance (as it would no longer be implied)

Other usual issue fields are not applicable - I spotted it during reviewing https://github.com/rubocop-hq/rubocop/blob/master/config/default.yml
matkoniecz added a commit to matkoniecz/matkoniecz-ruby-style that referenced this issue Jan 8, 2019
@matkoniecz
disable performance cops
e35f012 
ones that should be lint cops (or maybe could be) are skipped
promoting them to lint cops would be a good idea - first proposal is at rubocop/rubocop#6637

in general performance cops are making code less readable, applying them risks bugs (or eats too much time), code is more confusing...

overall the better way to improve performance is to run profiler

hopefully really useful performance fixes are done at language level
@matkoniecz
Copy link
Author

#6636 is an example of pull moving between categories

@matkoniecz matkoniecz changed the title move Performance/LstripRstrip to Lint/LstripRstrip promote Performance/LstripRstrip to Lint/LstripRstrip Jan 8, 2019
@Drenmi
Copy link
Collaborator

Drenmi commented Feb 5, 2019

Thanks for pointing this out, @matkoniecz. 🙇 Unless there's any unexpected behaviour or Ruby warnings from using #lstrip + #rstrip, it should go in the Style department, but moving it out of Performance seems like a good idea nonetheless.

@Drenmi Drenmi added enhancement good first issue Easy task, suitable for newcomers to the project labels Feb 5, 2019
@matkoniecz matkoniecz changed the title promote Performance/LstripRstrip to Lint/LstripRstrip promote Performance/LstripRstrip to Style/LstripRstrip Feb 5, 2019
@anuja-joshi anuja-joshi mentioned this issue Apr 1, 2019
Merged
anuja-joshi pushed a commit to anuja-joshi/rubocop that referenced this issue Apr 1, 2019
[Fix rubocop#6637] promoted Performance/LstripRstrip to Style/LstripR…
4e632e6 
…strip

Updated docs for Performance/LstripRstrip to Style/LstripRstrip promotion
@bbatsov bbatsov closed this as completed in 62aa383 Apr 1, 2019
@matkoniecz matkoniecz mentioned this issue Apr 1, 2019
Closed
@volmer volmer mentioned this issue Apr 4, 2019
Merged
jonabc added a commit to github/licensed that referenced this issue Apr 5, 2019
@jonabc
pin rubocop to 0.66
5c33c50 
until rubocop-github updates for rubocop/rubocop#6637
@jonabc jonabc mentioned this issue Apr 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement good first issue Easy task, suitable for newcomers to the project
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@matkoniecz @Drenmi