OPTION 1: Address regression introduced when running on Flog 4.9.0 #552
Conversation
|
@etagwerker -- If we take this fix then we don't need #551, and vice versa. But we do need one of them (or a different fix entirely). |
faisal
force-pushed
the
dont_freeze_flog_DEFAULT_OPTIONS_hash
branch
from
dbbaca2 to
9f3ceae
Compare
faisal
changed the title
|
My team ran into this issue, and after some review, I think this is the best fix. 👍 |
zenspider
left a comment
zenspider
left a comment
There was a problem hiding this comment.
added some critique to reduce the change needed... I'm prolly gonna dup on my end at some point since I think frozen hash literals are somewhere in our future
| methods: true | ||
| }.freeze | ||
| } | ||
| # rubocop:enable Style/MutableConstant |
|
|
||
| def initialize | ||
| super(DEFAULT_OPTIONS) | ||
| super(DEFAULT_OPTIONS.clone) |
There was a problem hiding this comment.
#>>> {}.freeze.dup.frozen?
# => false
#>>> {}.freeze.clone.frozen?
# => true
#>>> {}.freeze.clone(freeze: false).frozen?
# => false
|
My take: I don't agree with robocop that all literals should be frozen, but I don't have a problem with it in this specific case. But I think calling |
We freeze the DEFAULT_OPTIONS hash before sending it to Flog's initializer, but Flog 4.9.0 tries to modify it, causing multiple tests to fail. This patch changes the initializer to clone DEFAULT_OPTIONS to a non-frozen hash.
faisal
force-pushed
the
dont_freeze_flog_DEFAULT_OPTIONS_hash
branch
from
9f3ceae to
13d634b
Compare
|
Thank you for the investigation. I used clone(freeze: false) for clarity. |
|
@etagwerker |
Not speaking for the project or for @etagwerker here: We've been discussing how to proceed with releases. In the meantime, it looks like @zenspider changed how Flog deals with the options hash in recent Flog updates, and in my testing that got Rubycritic 4.11.0 working again. Are you able to update Flog? |
|
Thanks for your information!
I updated Flog, and rubycritic works again. |
5 participants
This is a fix to #548
This should actually fix the problem, in contrast to #551 which only works around it by limiting how far forward we pick up clog and flay versions. WE SHOULD ONLY TAKE ONE OF THESE CHANGES.
Details (courtesy of @neckhair): We freeze the DEFAULT_OPTIONS hash before sending it to Flog's initializer but Flog 4.9.0 then tries to modify the hash, resulting in #548. Following @zenspider's comments (see below), this PR patches the initializer to use a non-frozen clone of DEFAULT_OPTIONS instead.
Check list: