-
-
Notifications
You must be signed in to change notification settings - Fork 501
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
Reduce object allocations by using sort_by! where it is safe #924
Reduce object allocations by using sort_by! where it is safe #924
Conversation
The lists are not long and this happens only during parser initialization. Is there a reason why you think we need this optimization? If you want to optimize, I'd prefer a more profile-driven approach instead of applying micro optimizations. |
That was simply detected and applied by static analysis. |
Okay, I am fine with such changes as long as we add the linter and/or the static analyzer to the CI pipeline. Which one did you use? |
In the other PR you proposed to add rubocop to the CI. I'd appreciate that. |
I used the configuration we use @levups in CI : it uses Rubocop with Standard Ruby rules as its base, plus uses rubocop-capybara, rubocop-minitest, rubocop-performance, rubocop-rails, rubocop-rake, rubocop-thread_safety (not all relevant for this project). I also activated unsafe cops corrections + cops that are disabled by default (like those for performance). That should not be default but by reading warnings you can find some. |
We then need to agree on the code style you wish to use. Using Standard Ruby is a standard, sane, no-configuration option. Using a custom Rubocop configuration that matches the current code style is another one. To run Rubocop / Standard on CI and comment on PRs we use Pronto via GitHub Actions. Output looks like this : I can apply it to this repo. |
The standard configuration is likely a good starting point for the CI. I am not sure if using pronto is a good idea in the longer run. I prefer to add "plain" rubocob to the actions file without an additional middleman. |
Pronto is a heavy dependency, but having a pull request with comments in it
highlighting the issues can be a real productivity boost compared to
opening the Rubocop logs in a GitHub Action.
|
That's a good point. I will take a look at it. |
Calling
.keys()
always returns a new array, so we can usesort_by!()
to prevent allocating a new array.