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

Don't fail if the theme doesn't have a data dir #36

Merged
merged 2 commits into from
Mar 25, 2020

Conversation

fauno
Copy link
Contributor

@fauno fauno commented Mar 23, 2020

When the theme doesn't have a data dir the File.join fails because the path is nil

@ashmaroli
Copy link
Owner

@fauno I'm not against this change but would like to know why one would use this plugin if the theme they're using doesn't have a _data directory at theme-root..?

@fauno
Copy link
Contributor Author

fauno commented Mar 24, 2020

hi! so i'm building a self managed control panel for jekyll and one of the features is that you can exchange themes. i needed jekyll-data so i can load locales and other data templates from the theme (thanks!)

i was testing changing from minima to a custom theme, but jekyll-data would fail because minima doesn't have a _data directory. in your case, maybe a user is testing the plugin with such a theme? without this patch the build fails entirely.

@ashmaroli
Copy link
Owner

Thanks for sharing your use-case.

Okay, regarding your implementation here, I don't think you need to add that safe navigation operator in method :inspect_theme_data if there are no data files in the theme..
If you look at method :read_theme_data, you'll see that the whole definition is wrapped in a conditional.

I haven't tested this branch at my end yet, so if you think the safe navigation operator is necessary, and I missed something, do let me know.

@fauno
Copy link
Contributor Author

fauno commented Mar 24, 2020

no, you're right. i did wonder why read_theme_data was conditional but @theme_data_files wasn't :)

@ashmaroli
Copy link
Owner

Thank you @fauno!!

@ashmaroli ashmaroli merged commit e2490f2 into ashmaroli:master Mar 25, 2020
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.

2 participants