Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "instance variable @options not initialized" warning in verbose mode #650

Merged
merged 1 commit into from
Mar 25, 2018

Conversation

kolen
Copy link
Contributor

@kolen kolen commented Feb 28, 2018

When running in verbose mode (-w or --verbose or $VERBOSE = true), when instantiating any renderer (subclass of Redcarpet::Render::Base) without parameters, warning is shown about @options not initialized:

warning: instance variable @options not initialized

(For example, it can be seen if running RUBYOPT=-w rake test)

It's caused by accessing @options with rb_iv_get:

if (rb_iv_get(self, "@options") == Qnil)
rb_iv_set(self, "@options", rb_hash_new());

Replaced it with rb_attr_get, according to source, it's the same as rb_ivar_get but without warnings and without conversion of undef to nil, not sure if it's the right method by design (seems that it is).

Also not sure what is purpose for @options in render objects: there are no reads of it and all options are passed to actual renderer in constructor only, overwriting @options later is meaningless. Redcarpet::Markdown object adds its options to it when initialized but I can't figure out where these options are used (and these options are not for renderer but for "extensions"). Update: it's feature #560 and intended for custom renderers to be able to access "extension options" and not only renderer options passed in constructor.

I can try to detect warnings in tests, but I'm not sure if it's right idea (it can only be done in quirky ways such as capturing stderr), so not changed tests for now.

Copy link

@fedotov2d fedotov2d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@kolen kolen force-pushed the fix-warning-options-not-initialized branch from 238fa7d to 6b86656 Compare March 7, 2018 15:56
@robin850 robin850 merged commit e23383e into vmg:master Mar 25, 2018
@robin850
Copy link
Collaborator

Thank you very much @kolen ! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants