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

Take preferece of tags over branches when selecting the stable version #3331

Merged
merged 9 commits into from Nov 29, 2017

Conversation

@humitos
Copy link
Member

@humitos humitos commented Nov 29, 2017

I changed a couple of things here.

  • add more docstring to some methods
  • apply new style rules to modified files
  • add a debug package at testing environment

Let me know if you dislike this from this PR and I will remove them.

The interesting commit with the new logic to take a look is fd0b406

I will add more tests for this new logic.

Closes #3268

humitos added 2 commits Nov 29, 2017
This allows us to have the same `verbose_name` for different type of
versions (for example, '2.0' could both a tag and a branch). If this
happen, we want to keep them both.

In the UI, it will say just `"2.0"` for the branch and
`"2.0 (c0d25453)"` for the tag. This could be improved later if
necessary.
@humitos
Copy link
Member Author

@humitos humitos commented Nov 29, 2017

I just updated this PR with more logic on the sync_versions function and test cases for this.

Copy link
Member

@ericholscher ericholscher left a comment

Looks good!

@humitos
Copy link
Member Author

@humitos humitos commented Nov 29, 2017

@ericholscher I found another place where this PR is kind of related, can you check my commet at #3268 (comment) and give your opinion?

humitos added 2 commits Nov 29, 2017
Now, this logic is like this:

- if the project has any Version that is a TAG, the highest version
  will only be a TAG also (BRANCH are not considered)
- if the project doesn't have any TAG the highest version will be a
  BRANCH (or UNKNOWN in the worst case)
@humitos
Copy link
Member Author

@humitos humitos commented Nov 29, 2017

I just added one more commit here to follow what it was suggested in the comment and some tests for these scenarios at: bcfbca8

Can you please re-review? :)

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Nov 29, 2017

Ah yea -- the "version checking" code that is super old and janky :)

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Nov 29, 2017

That commit looks good! <3 getting these bugs fixed.

@ericholscher ericholscher merged commit 73b693c into master Nov 29, 2017
2 checks passed
@willingc
Copy link
Contributor

@willingc willingc commented Nov 29, 2017

Thanks @humitos 👍

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Nov 29, 2017

Should be deployed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants