Few more readability refactors #305
Conversation
Sija
force-pushed
the
Sija/followup-to-pr-300
branch
from
18e3dfd
to
e54029d
Compare
| json.object do | ||
| json.field :rule_name, rule_name | ||
| json.field :severity, severity | ||
| json.field :message, message | ||
| json.field :location, | ||
| {line: location.try &.line_number, column: location.try &.column_number} | ||
| json.field :end_location, | ||
| {line: end_location.try &.line_number, column: end_location.try &.column_number} | ||
| end | ||
| { | ||
| rule_name: rule_name, | ||
| severity: severity, | ||
| message: message, | ||
| location: { | ||
| line: location.try &.line_number, | ||
| column: location.try &.column_number, | ||
| }, | ||
| end_location: { | ||
| line: end_location.try &.line_number, | ||
| column: end_location.try &.column_number, | ||
| }, | ||
| }.to_json(json) |
There was a problem hiding this comment.
could you elaborate on the benefits of this change? Do we really need to create an intermediate hash object to make a json object?
There was a problem hiding this comment.
- readability
- consistency (it's the style being used in other places in the code)
and yes, it's allocating throw-away objects, but they're named tuples and not hashes, thus allocated on the stack, without putting pressure on the gc.
There was a problem hiding this comment.
To be honest #1 is subjective. In my opinion, the previous version is readable.
For #2 (consistency), the only way to guarantee it is to enforce it through a rule. Otherwise, (as it often happens) a new developer/maintainer comes and introduces discrepancies or tries to enforce other subjective rules re-writing the whole codebase.
I'm open to do these changes, but just want to be sure the style is preserved in a long term (if we want to follow it).
There was a problem hiding this comment.
I agree with both comments.
For #2 (consistency), the only way to guarantee it is to enforce it through a rule. Otherwise, (as it often happens) a new developer/maintainer comes and introduces discrepancies or tries to enforce other subjective rules re-writing the whole codebase.
While I do agree that would be the ideal solution, I suggest to leave it for a followup PR.
Followup to #300