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

Patch the baseline test new-year race condition. Fix #3723. #3724

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aknrdureegaesr
Copy link
Contributor

@aknrdureegaesr aknrdureegaesr commented Jan 1, 2024

Pull Request Checklist

  • [✔] I’ve read the guidelines for contributing.
  • I updated AUTHORS.txt and CHANGES.txt (if the change is non-trivial) and documentation (if applicable). Not applicable.
  • [✔] I tested my changes.

Description

Have the baseline test patch the current year to the copyright notes.

Once at it, I also improved error handling and traceability somewhat.

# In-place edit of copyright notes to adjust the copyright year.
import time
YEAR = str(time.gmtime().tm_year)
for edit_me in [
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn’t hardcode a list of files, it may change and cause the code to crash.

I would also prefer to use sed (with a slightly longer fragment to be sure we’re replacing the right thing) instead of doing it in Python via string manipulation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hard-coded list should or at least could be fixed. I agree to that.

Probably, the whole "patch" approach is not ideal.

What would you think about this idea: When the --invariant switch is given, the year should not be pulled from time.gmtime().tm_year, but instead some fixed number is used.

But then: Do you think this is worth it in the grand scheme of things?

It is sort of a reflex of mine to try to remove sources of change. My way of doing things: If Nikola version x.y.z once passed all the tests, I tend to fight hard so it will continue to pass all tests forever.

But that is only my preference. It does not seem to be this project's philosophy. For one, you do not pin dependencies, not even to major numbers.

So, what do you think? Is #3723 worth being fixed?

You seem to suggest, it's not.

Copy link
Member

Choose a reason for hiding this comment

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

I think the solution with --invariant would be a good solution to #3723 and one I would accept. This could be done by exposing a function somewhere in Nikola (utils.py?) and importing it in the sample conf.py file.

For one, you do not pin dependencies, not even to major numbers.

Pinning dependencies makes things difficult for downstream distributors, and there are a handful. We generally don’t support versions beyond the latest, and we don’t run tests for things other than the master branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.

TMI, in case you are interested: Now retired, I lived through the age of big enterprise software some 20 years ago, with a new production release every three months only, on a schedule. That official picture was only half-true in reality: We had production bug fixes much more frequently. In that world, it was very important that all automated tests that you ran at official release time would still pass at bug fix time 10 weeks later. The bug-fix build should only change the handful lines of sources we consciously had changed. You bet we fixed dependencies with all the nails and glue we could get our hands on! - 'twas a different world from what Nikola is facing today.

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