You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Add fix for lexer_options potentially being nil (#94)
The ruby Hash#delete function will remove a value by key from the
hash and return the associated value:
https://ruby-doc.org/3.4.1/Hash.html#method-i-delete
Where there is no value it'll return nil.
It seems that normally this is not an issue because
Middelman::Syntax::Highlighter sets :lexer_options with a default {}
in line /lib/middleman-syntax/extension.rb:16
However some folks might be calling super, might have changed
Highlighter.options or otherwise mucked around with opts or options
in a way that makes the assumption of presence here unreliable under
some circumstances.
There are likely a few ways to deal with this, whilst highlighter_options
gets passed further down, I can't find any evidence that the key :lexer_options
gets called with any assumptions beyond that point. So it might just be safe
to replace .delete with .dig here.
However as I'm not a maintainer here, I thought I'd suggest the minimum viable
change which is to add a guard clasuse that overrides the line if `nil` is returned
(suggesting the :lexer_options key wasn't present) and passes an empty hash instead.
This should prevent Lexer throwing an error on initalisation, and allow other
functionality to continue as normal.
I tried to figure out a test that proves this case, however all attempts seemed to
really go against the testing flow of the repo (for example by adding unit tests
and not just relying on feature tests), or found a struggle with overriding the default
{} set for lexer_options set in line lib/middleman-syntax/extension.rb:16.
Which leads to the interesting question of, how have a triggerd this bug in the first
place? As far as I can tell it's because we call `super` on the class and muck around
a bit from within our own repo.
So the slightly awkward test here is to use Gemfile :path to test this against our
repo. Which works. Sorry to not have added a better regression, but perhaps would
need a bit more of a talk about approach to best surface an issue like this.
Will leave it to maintainers to decide if that's good enough or if we should figure that
out before merging this.
0 commit comments