michaelherold / benchmark-memory

Memory profiling benchmark style, for Ruby 2.1+
MIT License
217 stars 6 forks source link

RuboCop improvements #16

Closed AlexWayfer closed 3 years ago

AlexWayfer commented 3 years ago
AlexWayfer commented 3 years ago

As this PR is inspired by #11, @dblock can be interested in.

michaelherold commented 3 years ago

Merged via 7ee4886.

Thanks for the contribution, @AlexWayfer!

As a bit of friendly advice, I suggest asking before making sweeping style-related changes to projects. The way you rewrote a bunch of tests made it harder to track the history of the file. Change history is much more important than adhering to draconian style rules from an external tool. In particular, the BlockLength cop is :-1: on any test files that use RSpec because they are, inherently, long blocks due to how the tool works!

I appreciate the effort you put into the changes, so accepted most of them squashed into a single commit. I also took the liberty of bumping to the Ruby versions supported by the latest version of memory_profiler, which, now that I think about it, might have broken the Travis build. Next up, I'm going to look at the GitHub Actions branch, so I don't consider that a problem.

AlexWayfer commented 3 years ago

As a bit of friendly advice, I suggest asking before making sweeping style-related changes to projects.

It's a PR to open-source, you owe nothing to me, I understand risks before making changes. It's just a suggestion and can be discussion.

The way you rewrote a bunch of tests made it harder to track the history of the file. Change history is much more important than adhering to draconian style rules from an external tool.

  1. I don't think it's a lot of changes, but OK, it's subjectively.
  2. I can agree that change history is important, but I can't agree that it's very important. Especially I don't see dirt in 1–2 commits, there are possibilities to blame further.
  3. It's not just "an external tool" for me and many others, it's a community standard, also flexible (configurable), so we can deal with it in desirable way.

In particular, the BlockLength cop is on any test files that use RSpec because they are, inherently, long blocks due to how the tool works!

Yep, I can agree, that's why I'm adding spec/**/* into BlockLengths Exclude in almost every project. It's OK for me: RuboCop doesn't force styles for any of external gems (maybe only with plugins).

I appreciate the effort you put into the changes, so accepted most of them squashed into a single commit.

Once again: I don't afraid to remake my PR, remove some commits, change them, etc. So, if something — don't hesitate to ask, please, at the end — it's my proposal.