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

Fix outdated command with non-release installed #456

Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Dec 19, 2020

This fixes shards outdated to work correctly with non-release versions. Currently it reports every version as outdated which is not the latest release, even if the installed commit is ahead of the latest release.

Resolves #446

In the example from #446, it's pretty obvious: The installed version is head on master, so it's really the latest and should not be listed as outdated.

When the requirement is a branch, the intention is to track that path. So the highest available version is head of that branch.

If there is a tagged releases with a higher version number (independent of the requirement restriction), that should always be treated as latest.

Commit and tag requirements are complicated because they provide no context to depict intention. A typical use case would be to pin an unreleased commit in master.
For now shards outdated assumes available versions for a tag or commit requirement to be on master branch.
Further enhancements could look whether the commit is actually an ancestor of master. That could help identify a pinned commit not merged to master. But there's not really a point forward from there, anyways. So it's probably fine to leave it that way.

There is currently a limitation that the requirements can only be accessed for first level dependencies. shards outdated just grabs that from shard.yml. For transitive dependencies I couldn't find an easy way to access the dependency requirement because there can be multiple ones and the solver just merges them together.
This only makes a difference when a dependency has a dependency with a branch restriction. Then the special handling for reporing branch head as available version does not work.
This can probably be improved later, but for now the fix is good enough and doesn't need to cover all edge cases.

This has already proven very valuable in practice, because it removes a lot of noise from shards outdated.

end
end

it "outdated commit" do
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow why a commit requirement should be flagged as outdated. On a branch yes, but a commit is a fixed requirement. Even stronger than a version number. I think that commits restrictions should not appear as outdated.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's really no difference to a version restriction: If I pin a version, I want to use that version. If pin a commit, I want to use that commit. When there's a newer version or a newer commit, I want shards outdated to tell me about it. I can still choose to keep the pinned version/commit, but I'm aware that newer versions are available.

If we would say a commit restriction is never outdated because the restricted commit itself doesn't update, the same applies to version restrictions really. If I want version 1.0, it's not going to update either, newer releases have different names.

A typical use case for pinning a commit is if you depend on an unreleased feature. It's a more limited restriction than a branch and prevents shards update from choosing newer, possibly unstable commits which are untested and could introduce unwanted breaking changes.
In that scenario, I would still like shards outdated to tell me if there are newer versions available which I might want to check out. Maybe there's even a new release and I can drop the commit restriction.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it's true that outdated does not force you to update. In that sense I see how you mean is the same as version number.

I rarely use commit restriction. Branches restrictions yes and I value to pick outdated when new commits on that branch are available. I see tagged releases a stable channel, so there is an underlying timeline on tagged releases to me. This timeline is what gives meaning to the outdated here. And I don't translate this timeline to commits for some reason.

But we can see how this plays along, definitely.

Copy link
Member Author

Choose a reason for hiding this comment

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

The behaviour is certainly not ideal yet if the pinned commit is not in the default branch. That's why I left a TODO for that.
But once the check is implemented that pinned_commit is an ancestor of HEAD, this should be more precise.

@bcardiff bcardiff merged commit 2440f81 into crystal-lang:master Jan 21, 2021
@bcardiff bcardiff added this to the vNext milestone Jan 21, 2021
@straight-shoota straight-shoota deleted the fix/outdated-non-release-version branch January 21, 2021 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shards outdated lists branch: master as outdated
2 participants