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

Feature/warn only crystal version #496

Merged
merged 10 commits into from
Apr 21, 2021

Conversation

beta-ziliani
Copy link
Member

This PR changes the semantics of the crystal: field to be a "maintainers claim it works in these versions of the compiler", warning in case the current version is not listed. An empty field means *, as coded in PR #493 by @oprypin.

It is an implementation (by @bcardiff) of the proposal I made as a comment in PR #493.

src/cli.cr Show resolved Hide resolved
src/cli.cr Outdated Show resolved Hide resolved
Added TODO per suggestion of @Sija

Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

I have a few suggestions to improve wording. Apart from that, it lookgs good to me.

docs/shard.yml.adoc Outdated Show resolved Hide resolved
docs/shard.yml.adoc Outdated Show resolved Hide resolved
src/cli.cr Outdated
@@ -44,7 +44,8 @@ module Shards
self.skip_postinstall = true
end
opts.on("--local", "Don't update remote repositories, use the local cache only.") { self.local = true }
opts.on("--ignore-crystal-version", "Do not enforce crystal version restrictions on shards.") { self.ignore_crystal_version = true }
# TODO: remove in the future
opts.on("--ignore-crystal-version", "Kept for compatibility, to be removed in the future.") { }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
opts.on("--ignore-crystal-version", "Kept for compatibility, to be removed in the future.") { }
opts.on("--ignore-crystal-version", "Has no effect. Kept for compatibility, to be removed in the future.") { }

@straight-shoota
Copy link
Member

It seems this needs a formatter run.

beta-ziliani and others added 2 commits April 16, 2021 17:31
Incorporating wording suggestions by @straight-shoota

Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@beta-ziliani
Copy link
Member Author

I think we're good to go, once the CI passes let's merge this (pinging @straight-shoota)

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

I'm not sure the reverted crystal: x.y.z semantics are the final word, but we can figure that out afterwards. Since it's just a warning now, the implications are not much of an issue.

@beta-ziliani beta-ziliani merged commit 778875e into master Apr 21, 2021
@luislavena
Copy link
Contributor

Hello @beta-ziliani @bcardiff, possible a new release of shards with this change is made?

Not asking packages of Crystal 1.0.1 to be released, but at least having shards updates so impacted folks can use or point users to that manual upgrade until a new distribution of Crystal is released.

Thank you.

@oprypin
Copy link
Member

oprypin commented Apr 29, 2021

(literally speaking, it should be possible, as mentioned here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants