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

hotfix(version) : change version comparaison #26

Merged
merged 1 commit into from Sep 18, 2021
Merged

hotfix(version) : change version comparaison #26

merged 1 commit into from Sep 18, 2021

Conversation

LouisAyroles
Copy link
Contributor

@LouisAyroles LouisAyroles commented Sep 15, 2021

Changing the version verification to give the appropriate connection choices to the user.
In fact, I had deliberately falsified the test before the release so that the connection was offered to me when the version was not yet greater than 0.12.
Maybe it will be more efficient to add some tests in order to check if the right connection choices are available.

Copy link
Member

@erikmd erikmd left a comment

Choose a reason for hiding this comment

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

Thanks @LouisAyroles! Indeed, this looks a sensible change to do for now.

Merging now, as the CI is still green.

Anyway, as you suggested, a dedicated test should be added later on, probably after defining an auxiliary function in charge of testing which choices to put forth to the user.

Also (as a note to myself), I recall that this test should be changed:

  • Replacing _ ≥ 0.13 with _ ≥ 0.14 given the current release plan
  • Replacing (learn-ocaml-client-version) with something that gets the min of the server version and learn-ocaml-client's version.

@erikmd erikmd merged commit c368480 into master Sep 18, 2021
@erikmd erikmd deleted the fix_version branch September 18, 2021 13:46
@erikmd
Copy link
Member

erikmd commented Oct 3, 2021

FTR,

Replacing (learn-ocaml-client-version) with something that gets the min of the server version and learn-ocaml-client's version.

is now done thanks to:

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.

None yet

2 participants