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

docs: improve description of enable_jekyll=disable_nojekyll (see #130) #132

Merged
merged 3 commits into from
Mar 6, 2020

Conversation

Cito
Copy link
Contributor

@Cito Cito commented Feb 28, 2020

This assumes enable_jekyll is implemented as (the preferred) alias for disable_nojekyll (see #130).

@Cito Cito requested a review from peaceiris as a code owner February 28, 2020 18:11
README.md Outdated Show resolved Hide resolved
Copy link
Owner

@peaceiris peaceiris left a comment

Choose a reason for hiding this comment

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

Thank you!

After merging the implementation of enable_jekyll by me, this pull request will be merged into the master. I will release it as v3.5.0 and update the v3 tag.

@peaceiris
Copy link
Owner

Can this description confuse users who use Jekyll on Docker? Actually, they do not need enable_jekyll: true. Their pages process may be slow without .nojekyll.

@peaceiris peaceiris self-requested a review February 29, 2020 13:48
@peaceiris
Copy link
Owner

The current description mentions .nojekyll file, not the GitHub Pages built-in Jekyll.

I came to think that we do not have to refer to the role of the .nojekyll file...

@peaceiris
Copy link
Owner

How about disable_adding_nojekyll_file instead of enable_jekyll?

@Cito
Copy link
Contributor Author

Cito commented Feb 29, 2020

How about disable_adding_nojekyll_file instead of enable_jekyll?

That's clearer than disable_nokeyll, but longer and clumsy, and I still I don't like the double negation that forces you to "think around the corner" as we say in German.

When deploying, users only care about whether Jekyll processing is enabled or not, They don't actually care what semaphore mechanism is used to achieve this (the .nojekyill file). That's why I mentioned it only in the second paragraph for those to understand the exact details.

@peaceiris
Copy link
Owner

Thanks to your great work, the description became more clear! LGTM.

After merging the implementation of enable_jekyll by me, this pull request will be merged into the master. I will release it as v3.5.0 and update the v3 tag.

@peaceiris peaceiris linked an issue Mar 2, 2020 that may be closed by this pull request
@peaceiris peaceiris mentioned this pull request Mar 6, 2020
peaceiris added a commit that referenced this pull request Mar 6, 2020
Implementation of `enable_jekyll` option which is an alias for `disable_nojekyll`

- Issue #130 
- Pull Request #132
@peaceiris peaceiris merged commit b76751b into peaceiris:master Mar 6, 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.

proposal: rename disable_nojekyll to enable_jekyll
2 participants