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

promote Performance/RedundantSortBy to Style/RedundantSortBy #6871

Closed
matkoniecz opened this issue Apr 1, 2019 · 7 comments
Closed

promote Performance/RedundantSortBy to Style/RedundantSortBy #6871

matkoniecz opened this issue Apr 1, 2019 · 7 comments

Comments

@matkoniecz
Copy link

matkoniecz commented Apr 1, 2019

Unlike many other Performance cops Performance/RedundantSortBy 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

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

#6637 ("promote Performance/LstripRstrip to Style/LstripRstrip") is a closely related issue that was fixed in #6870 by @anuja-joshi

There are also Performance/UnneededSort, Performance/Sample, Performance/RedundantMatch that also improve (at least in my opinion) code quality, independent from performance improvements but I think that creating now separate issues for them is a poor idea (if that would be a good idea I may create a separate issue for each, I am planning to do it also if RedundantSortBy would be fixed without fixing three additional ones).

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 3, 2019

Performance/UnneededSort, Performance/Sample, Performance/Sample

Seems you made some mistake here.

@matkoniecz
Copy link
Author

Fixed.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2019

Performance/UnneededSort, Performance/Sample, Performance/RedundantMatch

I agree about the first two, but RedundantMatch should stay in Performance IMO.

@matkoniecz
Copy link
Author

I crossed it out in the report.

@bbatsov bbatsov closed this as completed in 32ed7d1 Apr 4, 2019
@matkoniecz
Copy link
Author

@bbatsov Is it a good idea to look over Performance cops again whatever there is still something worth rescuing? Or is it something that you did already during making 32ed7d1 ?

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2019

Well, I not that worried about those cops as they are not going to die. :-) Took a quick look at some of the others, but nothing seemed like an obvious choice to go to style. Let me know if you have any ideas.

@matkoniecz
Copy link
Author

matkoniecz commented Apr 4, 2019

RedundantMerge, RedundantSortBy, ChainArrayAllocation looks like candidates but I am not 100% sure, I am even less sure about RedundantBlockCall.

RedundantMatch was rejected.

Well, I not that worried about those cops as they are not going to die

I know, but I would be happy to have in optional performance add-on restricted to rules making code style worse, this way I can easily disable/enable them as a group.

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

No branches or pull requests

2 participants