Skip to content

Add Performance/ReadlinesEach Cop

Matthias Käppler requested to merge 209922-add-readlines-each-cop into master

Description of the proposal

Refs #209922 (closed)

We discarded the idea of a catch-all Cop in !27590 (closed) that would flag all uses of read and readlines, which read files into memory in full. However, while writing !27887 (merged) it occurred to me that there are certain uses of readlines that are always offenses, since they read all lines just to discard them again. These can always be safely replaced with the more efficient each or each_line of the IO type.

I've rolled that into a Cop. There was only a single occurrence in a spec, so nothing dramatic. However, this will still help to prevent such code from sneaking into prod in the future, and the signal/noise ratio (unlike in the original MR) should be much higher.

Because the change from file.readlines.each to file.each_line is completely mechanic as well, we can also provide an auto-correction, which we weren't able to do before.

Check-list

  • Make sure this MR enables a static analysis check rule for new usage but ignores current offenses
  • Mention this proposal in the relevant Slack channels (e.g. #development, #backend, #frontend)
  • [-] If there is a choice to make between two potential styles, set up an emoji vote in the MR:
    • CHOICE_A: 🅰
    • CHOICE_B: 🅱
    • Vote yourself for both choices so that people know these are the choices
  • The MR doesn't have significant objections, and is getting a majority of 👍 vs 👎 (remember that we don't need to reach a consensus)
  • [-] (If applicable) One style is getting a majority of vote (compared to the other choice)
  • [-] (If applicable) Update the MR with the chosen style
  • [-] Create a follow-up issue to fix the current offenses as a separate iteration: ISSUE_LINK
  • Follow the review process as usual
  • Once approved and merged by a maintainer, mention it again:
    • In the relevant Slack channels (e.g. #development, #backend, #frontend)
    • (Optional depending on the impact of the change) In the Engineering Week in Review

/cc @gitlab-org/maintainers/rails-backend

Edited by 🤖 GitLab Bot 🤖

Merge request reports