-
-
Notifications
You must be signed in to change notification settings - Fork 5k
feat(validate_config): add warning about timezone #5731
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
Open
stevenjoezhang
wants to merge
1
commit into
master
Choose a base branch
from
tz
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+20
−1
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Do we have to guess and warn about a mismatch here? E.g., running Hexo on CI may have a different timezone, intentionally.
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.
Yes that the warning will always be displayed in CI environments. However, I think it should be displayed.
As @stevenjoezhang mentioned in the above comment at #5720 (comment), my concern is that URLs may change.
Currently, URLs are always generated using the LocalTimeZone (machine's timezone) for date/time formatting. If we change it to use the Timezone from
_config.ymlfor generating URLs in the next major version, and users are unaware that their machine's timezone and the timezone in_config.ymlare inconsistent, the generated URLs could unintentionally change.However, I do think it might be acceptable to address this by including a migration guide in the release notes instead of showing the warning.
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.
What do you think about only giving this warning if this is not in CI? (In CI, there is nothing users can do anyway; they need to validate this on his/her local machine).
Also, since this only affects permalink patterns with time, we may emit this warning only when one of the permalink patterns uses time.
cc @stevenjoezhang
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.
@yoshinorin @SukkaW I believe that displaying a warning message is necessary both locally and in CI environments, because there is a possibility that links may change. Showing a clear warning is more reasonable than remaining completely silent. For example, if a user does not configure the time zone correctly and this causes the article links generated by CI to change, the issue can still be discovered by checking the CI logs.
I think we still have two tasks to complete:
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.
@SukkaW @stevenjoezhang
I agree with only showing the warning when the permalink is affected.
As for CI environments, I don't think there's a reliable way to detect them. Even if there were, it would add unnecessary implementation complexity, so I think always showing the warning is fine. Instead of suppressing the warning in CI environments, it could help to make the warning message clearer, for example by including a link to the relevant documentation or this discussion.