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

shift polymorphism to version_parts #21

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

msarahan
Copy link
Contributor

@msarahan msarahan commented Oct 6, 2019

Follows discussion from #20

I hope this captures what we discussed. It doesn't work yet. I haven't quite gotten it to compile. I was basing a lot on:

https://internals.rust-lang.org/t/simplifying-custom-comparison-and-hashing/5108

which has a demo at https://play.rust-lang.org/?gist=b4445e8d2ffe2e8fe99bce52ecf507d4&version=stable&backtrace=0

The key thing is that each comparison is really 2 comparisons: first a match of priority, which is a proxy for type checking, and the second is a match of the actual value. The priority is what establishes the "trump" behavior that we discussed elsewhere.

PEP440 version comparison is obviously unfinished, and these parts are not connected up to the Version class at all yet. I wanted to get simple tests working in version_part.rs first.

@msarahan
Copy link
Contributor Author

msarahan commented Oct 8, 2019

After a lot of wandering and some very nice help from people at https://users.rust-lang.org/t/general-version-comparison/33349, I have landed pretty close to where I started. The code is very similar to what you had before I messed with it, but with one important difference: the partial_cmp is now implemented on the enum, which should vastly simplify the necessary comparison code in the loop in Version. Also, the ordering within the enum is used as a primary ordering, and only when enums being compared match are their values compared (so no type mismatch problems, but perhaps new strange behavior if "priority" of parts across a version are not uniformly decreasing.)

This is still horribly tedious, and I'm sure there's a better way, but I've floundered a lot with the information present in enums. The enum unifies the type nicely, and it also conveys the ranking of components. I have found it seemingly impossible to retrieve an index value from the enum - using the match to back them out is horrible and definitely error prone, especially since it seems to need repeating for the sake of the ref in the partial_cmp functions.

@msarahan
Copy link
Contributor Author

msarahan commented Oct 8, 2019

PS: I'll squash this all up and also comment it much better soon (please don't merge the PR in the mess that it's in!) If you have tips/ideas for generalizing code and getting rid of those horrible match uses, they would be very helpful.

@msarahan
Copy link
Contributor Author

@timvisee I think this is ready for some more discussion. I've made some changes that differ from your original design, and if you don't like them, that's OK. The highlights are:

  1. make the parsing function be the only point of customization (no VersionManifest)
  2. Create a new enum value, Empty, for empty strings and friends. This means that no Version is ever 0 parts. The Empty value is necessary because implicitly assigning 0 now means that an empty string is higher than something like "abc". The Empty value is like a permanent loser - the bottom of the totem pole.
  3. Several of the test cases have changed logic because of the implementations here. Many of the error tests are now no longer errors. Lexicographic sorting means that dev and alpha are not equal. Your long strings like "version-compare 3.2.0 / build 0932" seemed to rely on ignoring text. That doesn't work anymore with the default (though there could be a parser that ignores text).

I think I'm close to a good start for Conda's purposes. If this PR is something that you see potential in, I can spend some more time on it. Please let me know your thoughts on these changes.

On the other hand, if it has gone in weird directions that you'd rather not go, thanks for helping me learn. I'll close this PR and probably move the code into the libronda repo, which will eventually be the python library that uses this stuff: https://github.com/msarahan/libronda

@coveralls
Copy link

Coverage Status

Coverage decreased (-11.8%) to 83.267% when pulling 6cdb473 on msarahan:polymorph_parts into cb9fb13 on timvisee:master.

@coveralls
Copy link

coveralls commented Oct 22, 2019

Coverage Status

Coverage decreased (-11.8%) to 83.267% when pulling 6cdb473 on msarahan:polymorph_parts into cb9fb13 on timvisee:master.

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