Improved error message for version restrictions

[beta-ziliani]

This PR introduces two minor changes as a palliative to #521 :

Output on a failing run when the release doesn't exist:

- molinillo (0.3.0): The closest available release to 0.3.0 is: 0.2.0. These are the latest tags: v0.2.0, v0.1.0.

Output on a failing run when the release is wrongly set (it has a v):

- molinillo (v0.2.0): The closest available release to v0.2.0 is: 0.2.0. These are the latest tags: v0.2.0, v0.1.0.

Output on a failing run when the tag is not a release:

- pqueue (0.2.0): The closest available release to 0.2.0 is: 0.1.0. These are the latest tags: v0.1.0, 0.2.0.

[straight-shoota]

Having this additional information is certainly helpful.
However, I'm wondering if this is the best way to make it available.

The initial error message could already contain some diagnostic information to help the user directly, instead of just guiding to run again with --verbose. IMO "run again with --verbose" is a UX anti-pattern and should be avoided if possible. It requires manual interaction which is at best annoying in an interactive shell session, but can be really hard to deal with in automated environments (such as CI).
If we have the ability and knowledge to provide some helpful information directly, let's try to do that without another round trip.

We know which shard failed to install, so we could show some basic information such as available versions. Maybe filter/highlight ones that are similar to the missing version, to help against typos?

I expect the verbose tag output with --verbose could be quite polluted when you have a couple of dependencies with hundreds of tags (wether they are releases or not). And I'm not sure it would be that helpful to have this expansive information about all dependencies, when you're probably only interesting in one (the one which failed to resolve).
So perhaps a better way might be listing available versions per shard? This would fit well with a shards info command (#86).

[beta-ziliani]

100% agree. I frankly don't know why my past self thought that run with --verbose was a good idea...

[beta-ziliani]

I talked to my past self, and I realized that the --verbose solution was a quick hack that I should've mentioned. Getting the relevant information required a bit more of work, which I did as a POC in e87e8c0.

Now it produces the following output:

Resolving dependencies
Fetching https://github.com/crystal-lang/crystal-molinillo.git
Unable to satisfy the following requirements:

- `molinillo (0.3.0)` required by `shard.yml`
For molinillo the available tags are: [v0.1.0, v0.2.0]
Failed to resolve dependencies

There's currently no filtering, so for shards with a large list of tags it will print everything. Note also the following: this output doesn't help to the problem in #521:

- `molinillo (v0.2.0)` required by `shard.yml`
For molinillo the available tags are: [v0.1.0, v0.2.0]

I guess we need to filter the version before even reaching molinillo: if a requirement starts with v, fail altogether since it won't ever succeed.

[straight-shoota]

I like moving in this direction 👍

It might still be a bit much to always print all tags, though. An overload of information is probably not very helpful.

Maybe a first step could be to filter only tags that are valid versions.
Even that can be quite a lot. The maximum number of versions known to shardbox is 208.

And then maybe some Did you mean? suggestions could be a good idea. Levensthein distance might actually work reasonably well to suggest similar versions. Although some tweaking might help to account for the hierarchical semantics of version numbers.

[beta-ziliani]

It might still be a bit much to always print all tags, though. An overload of information is probably not very helpful.

Yes, I mention as a problem that there's no filtering.

But, as said, this doesn't solve the original problem, and the output is as confusing as before worse.

[beta-ziliani]

A bit hackish to say the least, but at least it produces a better output:

• If the version is wrong in the .yml file (v0.2.0 instead of 0.2.0):

- `molinillo (v0.2.0)` required by `shard.yml`
For molinillo the closest available tag to vv0.2.0 is: v0.2.0

• If the version is not listed (0.3.0):

- `molinillo (0.3.0)` required by `shard.yml`
For molinillo the closest available tag to v0.3.0 is: v0.2.0

• Is the version is not fixed (>=0.3.0):

- `molinillo (>=0.3.0)` required by `shard.yml`
For molinillo the last available tags are v0.2.0, v0.1.0

I'm certain this won't cover all possible scenarios, but at least it will improve the situation.

[straight-shoota]

suggestion: This should probably be a single error message. The conflict information is just context for the version conflict error, not itself another error.

[beta-ziliani]

OTOH if some workflow expects the error output as before, changing that might break it

[straight-shoota]

If something depends on the full exact text of the error message, it should be expected to break.
We cannot possibly guarantee stability on that.

[straight-shoota]

suggestion: This branch is about version restrictions. We should not confuse that with tags.
It's an implementation detail that versions are represented as tags in the version control systems.

So the base set should be available_releases instead of available_tags. For a version restriction we don't care about all the tags, only the versions.

Also, I believe we need to explicitly handle the case that no versions are available.

[beta-ziliani]

Do you mean simply replacing tags for release in messages and function names?

[straight-shoota]

The main change would be to use resolver.available_releases instead of resolve.available_tags as source of available items for this branch. And then adapt the wording as well, of course.

I can make a patch against your branch to demonstrate what I mean.

[beta-ziliani]

But that won't work for what we want to do here. If you filter already the tags, then you don't get information to display the error

[beta-ziliani]

I'll think of an alternative

[beta-ziliani]

I use releases but also show tags

[straight-shoota]

I think the top level distinction must be based on the type of restriction in order to produce an appropriate error message. For example, if the restriction is branch: foo there's no point talking about releases.

case req
when Version
  # fetch available releases
  # check for close matches
  # check for similar refs that are not versions
when VersionReq
  # fetch available releases
  # check for close matches relative to the restriction
  # check for similar refs that are not versions
when GitBranchRef, HgBranchRef, FossilBranchRef
  # fetch available branches
  # check for close matches
  # check for similar refs that are not branches
when GitTagRef, HgTagRef, FossilTagRef
  # fetch available tags
  # check for close matches
  # check for similar refs that are not tags
end

[beta-ziliani]

Mmm... this is harder than thought:

• Version is used in very specific cases, and I don't think a levenshtein diff will be useful
• VersionReq has the "just this version" case which follows the patter of Version above. This is the only useful case.
• Any other VersionReq is hard to comply to "close match". What is close match of ">0.3.0, <0.3.0"?
• A failing branch fails at checkin, not at resolving, so won't be part of this case.
• Tags never fail??

So I'm tempted to leave it as is for now.

[beta-ziliani]

I wasn't considering properly the case when the failure is a conflict with different shards. I guess there we need to resort to the original error message, that is, filter the case when there's only one conflict.

[straight-shoota]

I'm having a hard time wrapping my head around what's the changes are doing. And especially why things are implemented the way they are.
Part of the reason is certainly that I'm not well versed in the codebase. And that we're missing specs for this behaviour entirely doesn't make it easier to understand the effects.

But it's inherently quite a challenge to be aware of all the invariants in the code. For example, keep track what are the conditions that lead to each of the variations of error messages.
Now I could ask for clarification on each individual question, but it's a lot. And overall I think it would be a helpful and straightforward improvement - to ease reviewing and future work on this code - if we could add comments explaining the reasoning directly in the code. Context and rationale are quite often not self explanatory, unfortunately.

@beta-ziliani Let me know if you want me to take this.