-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Assign commits to versions following their commit graph (fixes #70) #72
Conversation
Before a commit was assigned the version/tag that happened next in the timeline. This leads to versions referencing commits that are not ancestors of it (and therefor not part of this version). Instead the versions should contain only all commits that are ancestors, up until a new version is encountered.
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.
Just a few suggestions. I'm going to review the rest in my editor as I think I'll probably modify the code a bit. I'll add a review here once I'm confident about my changes 🙂
I went ahead and applied the suggestions, I hope you don't mind 🙂 Other changes are to maintain backward compatibility. The |
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.
Some more cosmetic changes.
OK, sorry for the spam haha, just two more comments for which I'd like to hear your opinion 😄 |
23df803
to
9a997e7
Compare
I added a commit, that changes how the previous version is found. As you can see in the test case "test_two_release_branches" it is a bit odd / unexpected, that the previous version of "unreleased" was 1.1.0 (instead of 2.0.0). And even stranger, that version 2.0.0 did not have a previous version. Thinking about it, in my opinion, it is best to use the highest semantic version, that is found in the commit graph. |
That is a good remark. I think I agree with you: all versions that are not already present in the changelog should be ordered semantically (semver) rather than chronologically (tag date). It's particularly important for the "compare with previous version" URL, since we probably only want to compare a version with its semantically previous one, not the chronologically previous 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.
Minor changes.
(If you can, try to avoid force-pushing, as it makes reviewing a bit more difficult 😉) |
Co-authored-by: Timothée Mazzucotelli <[email protected]>
Co-authored-by: Timothée Mazzucotelli <[email protected]>
Co-authored-by: Timothée Mazzucotelli <[email protected]>
I think, I addressed all review comments. Let me know, if there is something else to do for this MR. |
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.
LGTM, thanks!
This is my attempt at fixing #70.
The solution in this PR is different from #70, but in my opinion, it makes the code easier to read and understand.
The first solution had some issues with certain commit graphs (especially, when there are more than one previous version).
I made the following assumptions:
I'll need to test this a bit more (especially if the last assumption is OK), but wanted to open this PR to get an early feedback.
I removed the
Version.next_version
field. If it is better to keep it, I can reintroduce it.One of the tests has an ugly
sleep(1)
in it. Unfortunately Git only has second precision and without the sleep, the comit order ofgit log
is undetermined for commits that happen in the same second (and are not related).And one final not, this PR is based on #71.