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

proposal: rename disable_nojekyll to enable_jekyll #130

Closed
Cito opened this issue Feb 27, 2020 · 4 comments · Fixed by #132 or #143
Closed

proposal: rename disable_nojekyll to enable_jekyll #130

Cito opened this issue Feb 27, 2020 · 4 comments · Fixed by #132 or #143
Assignees
Labels

Comments

@Cito
Copy link
Contributor

Cito commented Feb 27, 2020

Thank you for providing this useful tool, I like it a lot.

But today I stumbled over a small issue that made me waste too much time finding the cause. The problem was that the deployment worked, but all the CSS files did not show up. It did not help that GitHub had outages today which affected GitHub actions and GitHub pages and made me believe this could have caused it. Then it took me much time to find that the problem were not CSS files by themselves, but the fact that they were located in a directory starting with an underscore. All such files and directories did not seem to be visible. That was strange because they were visible in other projects. Finally, I found that this happens when Jekyll is activated. And then I saw my mistake: I had set disable_nojekyll: true believing that would create the .nojekyll file and disable Jekyll, while the opposite was the case. The double negative somehow was too much for my little brain today.

So my proposal is to rename the disable_nojekyll option to enable_jekyll, avoiding the double negative that can cause confusion.

Also, the documentation is not very clear. It says:

By default, this action adds the .nojekyll file to only the master and gh-pages branches. When the file already exists, this action does nothing. To disable this behavior, we can set the disable_nojekyll option to true.

It is unclear here whether "this behavior" refers to the fact that it adds the file only to the master and gh-pages branches or to the fact that the action creates the file at all.

My suggestion is to clarify the documentation a bit:

By default, this action adds the .nojekyll file if publishing to the master or gh-pages branch, thereby disabling Jekyll. When the file already exists, this action does nothing. To disable this default behavior, thereby allowing Jekyll processing of your pages, you can set the disable_nojekyll (enable_jekyll) option to true. If you are missing files or directories starting with an underscore on your site, this is probably caused by unintentionally enabling Jekyll processing.

@peaceiris
Copy link
Owner

peaceiris commented Feb 28, 2020

Thank you for suggesting this, and I am sorry for confusing you.

Document Changing

By default, this action adds the .nojekyll file if publishing to the master or gh-pages branch, thereby disabling Jekyll. When the file already exists, this action does nothing. To disable this default behavior, thereby allowing Jekyll processing of your pages, you can set the disable_nojekyll (enable_jekyll) option to true. If you are missing files or directories starting with an underscore on your site, this is propbably caused by unintentionally enabling Jekyll processing.

I totally agree with your changes of the documentation. (My notes old docs)

Could you open your pull request to update the documentation? @Cito

About Renaming

So my proposal is to rename the disable_nojekyll option to enable_jekyll, avoiding the double negative that can cause confusion.

Maybe, it is friendly and understandable for users to add the enable_jekyll option. But, we should not rename it for backward compatibility of version 3 (v3). It is better to add the enable_jekyll option as an alias for disable_nojekyll, and we can put the enable_jekyll option on our README instead of the disable_nojekyll.

CC: @nicolas-van @dhimmel

@peaceiris peaceiris added the enhancement New feature or request label Feb 28, 2020
@Cito
Copy link
Contributor Author

Cito commented Feb 28, 2020

Thank you for the quick response. Sure, the disable_nojekyll option should still be kept for some time as an alias for backward compatibility. Will send a PR for the docu later today.

peaceiris added a commit that referenced this issue Mar 6, 2020
Implementation of `enable_jekyll` option which is an alias for `disable_nojekyll`

- Issue #130 
- Pull Request #132
peaceiris pushed a commit that referenced this issue Mar 6, 2020
#132)

This assumes `enable_jekyll` is implemented as (the preferred) alias for `disable_nojekyll`

Close #130
@peaceiris
Copy link
Owner

v3.5.0 has been released and v3 updated. Thanks.

@github-actions
Copy link
Contributor

This issue has been LOCKED because of it being resolved!

The issue has been fixed and is therefore considered resolved.
If you still encounter this or it has changed, open a new issue instead of responding to solved ones.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
2 participants