-
Notifications
You must be signed in to change notification settings - Fork 109
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
Update timestamps each time a build is done #772
Conversation
🤖 Pull request artifacts
|
What needs to be done to move this forward? |
If you confirm that this PR works properly, I will merge it |
Ahh, sorry. Didn't realize this was up to me. :-> |
Embarrassing git novice question (I've not done this step before): How do I get this locally using From reading https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally, I thought I could something like: But I just git an error:
|
This should work:
|
That's what I was trying but got the |
Thats because you are fetching from your fork, not the original repository. I'd really only suggest checking out the fix/build-timestamp branch. |
I'd just figured that out. Like I said, novice. :-/ |
Maybe, catch us on telegram, we can discuss these things better/faster there... |
Looks good to me. Thanks! |
settings.py
Outdated
) | ||
|
||
|
||
def build_timestamp(): |
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.
Would it be better or more readable to call these functions from the get_current_timestamp method and have the actual building here?
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 you propose to get rid of get_current_timestamp()
, move the code to build_timestamp()
and build_date()
?
Maybe. I like both options, there are pros and cons to both
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.
Both have a race condition. If the second ticks over between the calls to gmtime()
, the results will be different.
I don't know if there is any code that depends on those values being identical.
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 you propose to get rid of
get_current_timestamp()
, move the code tobuild_timestamp()
andbuild_date()
?
Maybe. I like both options, there are pros and cons to both
Well, no... do not remove the get_current_timestamp.
Rather to have the time.strftime(...)
inside of the "build" methods which would get called from get_current_timestamp with the same "time" parameter and have this parameter default to time.gmtime() in case its called separately.
The point is to not calculate both when we need only one of those.
(This is just about polishing the code, I'm quite ok with the current state, it just feels weird to call method to calculate several values and then use only one 🤷♂️)
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.
No need to block the PR for this, but if it could be improved I'd be glad 😉
I've updated the code to get rid of a possible time unsynchronization within the same build |
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 just remove the return from generate timestamp, otherwise ok
3d53541
to
dd3877b
Compare
Thanks! |
Found a way to avoid code duplication
Closes #740 #770