search

crystal-lang / shards

Dependency manager for the Crystal language
Other
758 stars 99 forks source link

Fix: make 'v' prefix optional #626

Closed devnote-dev closed 1 month ago

devnote-dev commented 2 months ago

Closes #521.

ysbaddaden commented 2 months ago

Thanks!

An immediate issue is that there are no tests to ensure that the resolvers do support the new kind of version tags (install might be broken), or that they can handle a mix of them from different shards (probably not an issue).

I suppose edge cases should also be handled: what if both tags exist (with the current patch they will both be listed)? won't Molinillo break with two version tags? what if they point to different commits? should that be an error? should one form take priority (to add to the confusion).

Another issue is that sites such as https://shardbox.org and the ecosystem will now have to support both kinds, instead of assuming a convention.

straight-shoota commented 2 months ago

Yeah, this needs a lot more work to ensure everything is working correctly. So far there has not been any talk about these details because #521 hasn't come to any conclusion whether this is actually wanted. I think we should continue the discussion there.

devnote-dev commented 2 months ago

An immediate issue is that there are no tests to ensure that the resolvers do support the new kind of version tags (install might be broken), or that they can handle a mix of them from different shards (probably not an issue).

@ysbaddaden I'm not sure I understand what you're asking for here — the prefix was a hard requirement prior to this meaning resolvers had to support it, all this PR does is remove said requirement which wouldn't affect anything.

I suppose edge cases should also be handled: what if both tags exist (with the current patch they will both be listed)? won't Molinillo break with two version tags? what if they point to different commits? should that be an error? should one form take priority (to add to the confusion).

This isn't changing any core functionality of the resolver. If both v0.2.0 and 0.2.0 exist, the resolver will pick the version that is specified. Version mismatches (i.e. meaning to pick one over the other) ultimately comes down to a user error which is beyond the scope of Shards.

Another issue is that sites such as https://shardbox.org/ and the ecosystem will now have to support both kinds, instead of assuming a convention.

Not sure how that plays into this? If Shardbox also faced this issue then it should have been something addressed on their end.

So far there has not been any talk about these details because https://github.com/crystal-lang/shards/issues/521 hasn't come to any conclusion whether this is actually wanted.

@straight-shoota I opted for this solution because it's easier to implement (literally a one line change) and aligns with other package management tools. Changing the error message to be more user-friendly was the original suggestion but Shards' codebase is annoyingly ambiguous regarding typings and its order of operations (highlighted in #574).

ysbaddaden commented 2 months ago

I'm not sure I understand what you're asking for

The test suite verifies that version tags starting with a v still work; they don't test that version tags without a v are working. We need a set of tests to prove that they work.

This isn't changing any core functionality of the resolver

Having both v0.2.0 and 0.2.0 means that we'll have the 0.2.0 version twice from different tags. In the lucky case they're duplicates and it's fine (though we should at least de-duplicate), in the unlucky case they're different commits with different shard.yml and we must have a rule to handle this edge case (e.g. skip both tags and warn about it).

devnote-dev commented 2 months ago

The test suite verifies that version tags starting with a v still work; they don't test that version tags without a v are working. We need a set of tests to prove that they work.

As far as I can see the resolver specs don't test for "v" so I'm not sure what to change there.

in the unlucky case they're different commits with different shard.yml and we must have a rule to handle this edge case (e.g. skip both tags and warn about it).

I'm not sure where exactly this would go and I'm not having any issues reproducing it locally.

crysbot commented 1 month ago

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/shards-mad-at-me/6882/4

straight-shoota commented 1 month ago

I'm closing this because it's an incomplete, trivial solution. There's no point in keeping this PR open and spreading the discussion across multiple threads.

Changing this behaviour is decisively not a one-line change. It involves other parts of the code base, the spec suite and other participants in the shards ecosystem at large.

We should continue the discussion in https://github.com/crystal-lang/shards/issues/521 about whether this change actually makes sense, and if so, figure out how to properly implement it.