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

Build spec module #305

Closed
wants to merge 22 commits into from
Closed

Conversation

YeungOnion
Copy link
Contributor

I think this is a little bigger than it needed to be, but I felt like I'd necessarily need to reimplement a slightly smaller version of everything but the tree in version_spec, so I wrote with some of those thoughts in mind.


As I write this I realize that I haven't pulled in a bit and didn't see the work on breaking out the VersionOperator into its parts. So a lot of what I was thinking to do is started up there. I'll move this to draft so I can submit this with some of this using what's existing in version_spec.

Would it be fine to change version_spec::parse::constraint_parse to support type T, then call it for VersionConstraint = Any | Comparison | StrictComparison | Exact and also call it for BuildConstraint = Comparison | Exact?

Or does that not make sense in the context of build specs?

is bulkier than needed, I know.
But this was written with intent to refactor version_spec to share some of this
aliased is_member to matches for consistency (Set should either have a callable or other uses of matches should return Fn -> bool
aliased BuildNumber type for u32
@YeungOnion YeungOnion marked this pull request as draft August 30, 2023 21:33
@baszalmstra
Copy link
Collaborator

Starting to look good already! Might indeed be a little bit over-engineered but that's fine. :)

@YeungOnion If you can make it generic that would be great yeah!

should probably add more tests
…pturing both

motive is that this is already implemented for build spec
this couples eq and ord since VersionOperator already couples those together
@YeungOnion YeungOnion marked this pull request as ready for review September 7, 2023 18:01
Version::from_str("1.2.3").unwrap()
),
VersionSpec::Range(RangeOperator::Less, Version::from_str("2.0.0").unwrap()),
VersionSpec::OrdRange(OrdOperator::Ge, Version::from_str("1.2.3").unwrap()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like fmt not working here

@YeungOnion
Copy link
Contributor Author

Think in my overzealousness and irregularity on working on this, I missed this and I'm not sure if I deprecated any feature there. I don't see a diff where I move StrictVersion to Version, but I'm not sure if StrictVersion is a different type or it was a way to specify that this Version would have one of the StrictRangeOperators, if so, I'd like to hear about the background for that decision as it seems duplicative to me, but I don't have the context @tdejager when they added those other operators and StrictVersion

unsure how to direct test build number from match spec
@tdejager
Copy link
Collaborator

tdejager commented Sep 8, 2023

Hi @YeungOnion this was added to support the case where a Version like 1.0.0 != 1.0. Normally, trailing zero's on a version are considered the same as a version without trailing zeros. This if fine normally. Only for the case for startsWith and compatible it is not. Because in that case you want to start the range for a specific version.

The range 1.0.* is unequal to the range 1.*. And because 1.0.0 and 1.0 were considered the same version. This caused problems when using Hash for example. Because these values: tensorflow 2.0.* and tensorflow 2.* would hash to the same values. This is no longer the case when using the StrictVersion type.

While in most cases like an equality of a single version v1.0 == v1.0.0 the versions can be considered the same. Also for a greater than, like > 1.0 you do not care about the trailing zeros either so these all hash to the same values, and are equal. E.g >= 1.0 == >= 1.0.0, because they both equal to the same range

Hope that this gives the needed context.

@YeungOnion
Copy link
Contributor Author

I think this gives a good chunk of the context. I still don't understand why the operator alone isn't enough, I do see that there's a different impl PartialEq for it that uses the number of components as well as the segment comparison.

Is it not used explicitly as part of the spec? I see that it's not used in the version constraint enum and I may be misunderstand where a user would specify that a spec is for a StrictVersion

@YeungOnion
Copy link
Contributor Author

Separate meta topic: I think my PR got far too big.

Looking for feedback or tips for not doing things like this in the future. As you can see, I've not the practice or discipline of incremental progress yet.

@YeungOnion
Copy link
Contributor Author

I'll start a few new PR's as this one modified more than it needed to at once. I also cut back on some unneeded generality.

@YeungOnion YeungOnion closed this Sep 15, 2023
@YeungOnion YeungOnion deleted the build_spec_module branch September 16, 2023 01:18
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.

3 participants