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 shard crystal version in crystal init #13730

Merged

Conversation

xendk
Copy link
Contributor

@xendk xendk commented Aug 6, 2023

The crystal: x.y.z format is described as legacy and discouraged in the shards docs, so update it to use the version constraint.

Don't use legacy format.
@@ -10,6 +10,6 @@ targets:
main: src/<%= config.name %>.cr

<%- end -%>
crystal: <%= Crystal::Config.version %>
crystal: '>= <%= Crystal::Config.version %>'
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't more conservative ~> be safer by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't really consider alternatives, I just made it explicitly use the constraint that the shards docs says it uses for plain versions:

There is a special legacy behavior (its use is discouraged) when just a version number is used as the value: it works exactly the same as a >= check: x.y.z is interpreted as ">= x.y.z"

But now you mention it:

I created a project back in April (according to the file timestamps), which added crystal: 1.7.3. So if the using the ~> constraint, it would already be outdated by two minor releases.

My feeling is that the intention of most users would be to be compatible with all crystal versions until the next major version.

Copy link
Contributor

Choose a reason for hiding this comment

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

My feeling is that the intention of most users would be to be compatible with all crystal versions until the next major version.

If so, then the proper constraint would be ~> x.x.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I disagree, but that's a change of behavior, not just fixing it to not use a discouraged format.

It's also a tad more involved. Currently, it's just using Crystal::Config.version which is the full version. To get the version needed for ~> we'd need either some string hackery (which seems wrong to me) or extend SemanticVersion with a to_s-like method to render the given version with a given granularity, so we can do something like SemanticVersion.parse(Crystal::Config.version).bump_major.to_s(:minor).

But I'd prefer a nod from a core team member that this change is acceptable before trying my hands on an implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure thing, I agree it's a bit more involved solution, just mind you, that for the all crystal versions until the next major version part to hold, you'd need to use >= n.n.n, < n.n constraint, which IIUC is practically the same as ~> n.n.

@xendk
Copy link
Contributor Author

xendk commented Aug 16, 2023

Oh, but of course there's a test that needs fixing. I'll look at it tonight.

@xendk
Copy link
Contributor Author

xendk commented Aug 16, 2023

There.

@xendk
Copy link
Contributor Author

xendk commented Sep 11, 2023

Oh, I didn't notice the failure there... But doesn't seem to be related to the PR?

@straight-shoota
Copy link
Member

No, that failure is unrelated. Interpreter specs are flaky.

@straight-shoota straight-shoota added this to the 1.10.0 milestone Sep 22, 2023
@straight-shoota straight-shoota changed the title Fix shard crystal version in init Fix shard crystal version in crystal init Sep 24, 2023
@straight-shoota straight-shoota merged commit 71d613f into crystal-lang:master Sep 24, 2023
52 of 53 checks passed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
This pull request was closed.
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.

4 participants