Skip to content

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.

@LouisAyroles LouisAyroles requested a review from erikmd September 15, 2021 09:58
Copy link
Contributor

@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
Contributor

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.

2 participants