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 OTP version null check #211

Merged

Conversation

cgrothaus
Copy link
Contributor

@cgrothaus cgrothaus commented Jun 14, 2023

Fixes #210.

We need to check that the available versions contain the resolved version (which may be the original spec).

@paulo-ferraz-oliveira
Copy link
Collaborator

You need to npm run build-dist and make sure there's no code to commit (we have an action here that's most surely gonna fail). Check CONTRIBUTING.md for more details...

@paulo-ferraz-oliveira
Copy link
Collaborator

Also, it is possible your fix surfaced another bug (in the tests themselves)...

@cgrothaus
Copy link
Contributor Author

I slightly adapted the "available versions included the spec" check. More precisely, I adapted when that other branch that does the semver resolution magic is executed. From reading the existing code, I think that's the desired behaviour:

  • For release candidates or when using version-type: strict, we just use the spec as the exact version.
    • But only if it is in the list of available versions (<-- this is new)
  • Else do the semver resolution magic.

Thanks for the hint to CONTRIBUTING.md. I ran build-dist.

Regarding the unit tests, I don't get it why they fail when installing Gleam. I think this is coincidence, I don't see how it would be caused by the changes of this PR.

@cgrothaus
Copy link
Contributor Author

I kneaded it one more time. The inclusion check wasn't working when we need to prepend a 'v'. So I moved the check to the end of the function.

@paulo-ferraz-oliveira
Copy link
Collaborator

Could you (also) cook up a test that shows how effective the fix was? We use version coercion and guessing (based on SemVer rules) and I'd prefer to not let a bug such as "wrong" coercion/guessing slip through the cracks. Thanks.

@cgrothaus
Copy link
Contributor Author

I refactored to a single return and added a test case.

@paulo-ferraz-oliveira
Copy link
Collaborator

Seems Ok. Thanks for this. I'll merge and release later.

@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit df9a609 into erlef:main Jun 15, 2023
59 checks passed
@paulo-ferraz-oliveira
Copy link
Collaborator

Merged. Thanks.

@cgrothaus cgrothaus deleted the fix-otp-version-null-check branch June 15, 2023 14:41
@paulo-ferraz-oliveira
Copy link
Collaborator

Released in v1.16.0 / v1.16. Tag v1 moved up...

@cgrothaus
Copy link
Contributor Author

🆒 ! Thanks a lot.

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.

Function getOTPVersion does not catch the version being undefined
2 participants