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

Infer minimum Gleam version #3557

Merged
merged 16 commits into from
Sep 11, 2024
Merged

Infer minimum Gleam version #3557

merged 16 commits into from
Sep 11, 2024

Conversation

giacomocavalieri
Copy link
Member

This PR closes #3492

It's still a WIP because I have some questions before I can move forward:

  1. I used the Version type that comes from hexpm::version, should we use our own minimal custom type instead of relying on this one?
  2. If someone specifies an insufficient version (let's say gleam.toml has 1.2 but we inferred the minimum one to be 1.5) how should the error be presented to the user?
    Do we want the error message to include an example of a usage of a feature that explains why we inferred that number? For example something that looks like this:
    error: invalid Gleam version
    
    1 | tuple.0.1
             ^^^^
    
    Nested tuple access requires a Gleam version >= 1.2 but the version specified in
    your `gleam.toml` is 1.1!
    Even if someone used multiple features I think it would be better to show just one, I don't think there's much value in making the error message any longer
  3. What if the gleam.toml doesn't specify any gleam version? Do we want to manually add the constraint when publishing it or have an error and force the programmer to explicitly write the inferred version down? Is there any downside to doing it implicitly?

@lpil
Copy link
Member

lpil commented Aug 23, 2024

Heya! I've not looked at the code yet but here's my thoughts on the Qs:

  1. Hex one is fine.
  2. Warn for analysis, error for publishing. We show all warnings rather than stopping after the first instance as the programmer wants to know about all of them so they can fix it. Seems like it might be tricky working out what the requirement means as it can be of arbitrary complexity such as >= 1.0.0 and < 3.0.0 or >= 4.0.0 and < 5.0.0.
  3. Add our own when publishing seems fine to me.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Beautiful work as per usual!!!! 💜

@lpil lpil merged commit d04bd01 into gleam-lang:main Sep 11, 2024
12 checks passed
@giacomocavalieri giacomocavalieri deleted the fix-3492 branch September 11, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infer minimum Gleam version
2 participants