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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Interpreter warning from Jekyll::Renderer #7448

Merged
merged 1 commit into from Jan 3, 2019

Conversation

mojavelinux
Copy link
Contributor

Change OR operator to a lazy assignment in Jekyll::Renderer#layouts

This is a 馃悰 bug fix.

Summary

When warnings are enabled, Jekyll emits a rather insidious warning message (inidious because of how many times it occurs).

warning: instance variable @layouts not initialized

This patch fixes the problem.

Context

Based on other methods in the Jekyll::Renderer class, I believe the intention is to use a lazy assignment operator here instead of an OR operation.

If that's not the case, then I can update the patch to allow the OR operator to be used without emitting a warning.

Semver Changes

Patch.

ashmaroli
ashmaroli previously approved these changes Jan 2, 2019
Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

Good catch 馃憤
Note: Renderer has a :layouts= method to set the value of @layouts

@ashmaroli ashmaroli changed the title fix warning in render Fix Interpreter warning from Jekyll::Renderer Jan 2, 2019
@ashmaroli ashmaroli added the fix label Jan 2, 2019
@pathawks
Copy link
Member

pathawks commented Jan 2, 2019

I don鈥檛 think the goal here is memorization.

Looking back at Git history, we used to set @layouts in the initalizer. I wonder if it would be safer to just explicitly initialize it properly; that way we could still check layouts.nil?
(I don鈥檛 know how important that is; just thinking aloud.)

@mojavelinux
Copy link
Contributor Author

@pathawks That's the insight I was hoping for. I couldn't tell from the test suite which way to go here. All tests pass in either case.

If you'd like me to switch to just removing the warning (and not adding memoization), I can proceed accordingly (by setting the value in the initializer like you suggested).

@pathawks
Copy link
Member

pathawks commented Jan 2, 2019

I can proceed accordingly (by setting the value in the initializer like you suggested).

Yeah, let鈥檚 go that route instead. 馃憤馃徏

@ashmaroli
Copy link
Member

IMHO, there wouldn't be much of an issue because with this memoization, calling renderer.layouts is going to return the value of either @layouts or site.layouts anyway..

- assign a default value to @layouts in Jekyll::Renderer
@mojavelinux
Copy link
Contributor Author

Code updated.

@ashmaroli ashmaroli dismissed their stale review January 2, 2019 08:20

Diff changed..

@DirtyF
Copy link
Member

DirtyF commented Jan 3, 2019

Thanks @mojavelinux 馃憤

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit 9a0153f into jekyll:master Jan 3, 2019
jekyllbot added a commit that referenced this pull request Jan 3, 2019
@mojavelinux mojavelinux deleted the fix-warning-in-renderer branch January 3, 2019 01:40
@mojavelinux
Copy link
Contributor Author

Thank you all for reviewing!

@jekyll jekyll locked and limited conversation to collaborators Jan 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants