Fix Ensure check methods can't modify tokens array
#233
Conversation
alexjfisher
force-pushed
the
fix_duplicated_tokens
branch
from
2257895 to
ec82d69
Compare
|
After a bit more testing, it looks like |
|
lol |
The functionality originally released in rodjek#296 was probably broken at the time, possibly made worse by later rubocop 'fixes' and also incompatible with ruby 3.4. We now look at the stack trace only from the `tokens` wrapper method in `checkplugin`. Fixes puppetlabs#225
alexjfisher
force-pushed
the
fix_duplicated_tokens
branch
from
ec82d69 to
ff0337e
Compare
| @tokens | ||
| end | ||
| def tokens(duplicate: false) | ||
| duplicate ? @tokens.dup : @tokens |
There was a problem hiding this comment.
I had a crazy thought: rather than duplicating, would it make sense to freeze it? https://rubyapi.org/o/array#method-i-freeze says it will not do anything if already frozen, so should be cheap and you can't modify it.
There was a problem hiding this comment.
There wasn't much information I could find about to why this was originally introduced, but other methods (such as fix) definitely do need to be able to modify the array.
| end | ||
|
|
||
| def fix | ||
| tokens << :fix_token |
There was a problem hiding this comment.
I don't think this is supposed to be a valid API because there's also the add_token to actually modify it.
| def check | ||
| # Since we're calling `tokens` from a `check` method, we should get our own Array object. | ||
| # If we add an extra token to it, PuppetLint::Data.tokens should remain unchanged. | ||
| tokens << :extra_token |
There was a problem hiding this comment.
When returning a frozen array it will raise a FrozenError. Perhaps we should catch that and raise a deprecation warning?
The functionality originally released in rodjek#296 was probably broken at the time, possibly made worse by later rubocop 'fixes' and also incompatible with ruby 3.4.
We now look at the stack trace only from the
tokenswrapper method incheckplugin.Fixes #225