-
Notifications
You must be signed in to change notification settings - Fork 601
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
Require Hugo 0.120+, fix deprecation warnings #502
Conversation
Any idea of how far back the new one works? |
New in 0.120 https://gohugo.io/functions/hugo/isserver/ Did they have to deprecate the same version they added the replacement? |
Eh, we are on 0.128 now, so I'd be okay with making 0.120 the minimum, I think. Anyone else? |
I would say that being basically 8 versions ahead of the deprecated function is enough. I also found another deprecated feature .Site.DisqusShortname which also got deprecated since 0.120. If we change to |
In the face of that we will update to 0.120 it would also be nice to update the README to reference the hugo.* file as the config instead of the config.* file. I also changed the description of the disqus feature to match the updated version of the code
Can we make it a hard error (errorf) if too old? |
Could we use the min_version parameter in the theme.toml and HUGO_VERSION in netlify.toml file? |
Other than that we could maybe use the errorf and the |
The theme.toml one sounds good. I assume Hugo would error with a nice message if that was too high. |
@@ -1,7 +1,7 @@ | |||
{{ if (.Params.comments) | or (and (or (not (isset .Params "comments")) (eq .Params.comments nil)) (.Site.Params.comments)) }} | |||
{{ if .Site.DisqusShortname }} | |||
{{ if .Site.Config.Services.Disqus.Shortname }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we error if Site.DisqusShortname
is set? Similar to what I added in #481? Otherwise no one will know this was changed, it will just vanish from sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess you can add that. Sounds good.
Edit: I can also add it if you have not time for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd like to, go ahead. Pretty distracted at the moment with catch up in various projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok you can not access the disqusShortname
as you did with author
due to the fact that it is a default value in .Site
and will always be set by default to the empty string. As far as I see it there is not a proper way of checking if people use this deprecated feature without getting a warning because we need to use .Site.disqusShortname
to actually check if people use it. I would say a compromise is that we do a check in a central location using .Site.disqusShortname
with a warning that after sometime this check will be removed as .Site.disqusShortname
is deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea is to use this in the baseof.html
{{ if le hugo.Version "0.120.0" }}
{{ errorf "Please use Hugo version 0.120.0 or higher!"}}
{{ end }}
{{ if not (eq .Site.DisqusShortname "") }}
{{ errorf "Your use of disqusShortname is deprecated by Hugo; Please reference the README.md for further instructions.
This error message will be removed in the future because we are also using .Site.DisqusShortname for checking this."}}
{{ end }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable. I can move the other check (Site.Author) there too after it's in.
I did some tests and changing Maybe adding another check in something like the |
Would you mind rebasing or merging to resolve conflicts? Then I'll trigger the CI. |
layouts/_default/baseof.html
Outdated
|
||
{{ if not (eq .Site.DisqusShortname "") }} | ||
{{ errorf "Your use of disqusShortname is deprecated by Hugo; Please reference the README.md for further instructions. | ||
This error message will be removed in the future because we are also using .Site.DisqusShortname for checking this."}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are multiple lines allowed?
ERROR "/home/runner/work/beautifulhugo/beautifulhugo/layouts/_default/baseof.html:6:1": parse failed: template: _default/single.html:6: unterminated quoted string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is somewhere in the single.html, but I can't find where.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It says line 6 of baseof.html, which is exactly this multiline string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you need tick-marks instead? gohugoio/hugoDocs#764
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now
[services] | ||
[services.disqus] | ||
shortname = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[services] | |
[services.disqus] | |
shortname = '' | |
[services.disqus] | |
shortname = '' |
Probably matches everything else, so not changing it here, but I really don't like this fake TOML nesting. Pretending TOML cares about indentation when it doesn't isn't helpful, IMO.
Thanks for pushing this through! |
* Fixed deprecated site.isServer param to hugo.isServer param * Also fixed another deprecated function for disqus * Updated README.md In the face of that we will update to 0.120 it would also be nice to update the README to reference the hugo.* file as the config instead of the config.* file. I also changed the description of the disqus feature to match the updated version of the code * Added checks for hugoVersion and disqus feature * Fixed accidental delete * Removed new line
I changed the parameter .site.IsServer parameter to hugo.isServer because the first one is deprecated.