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

[Breaking] Change match_raises to be more intuitive #419

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

psafont
Copy link
Contributor

@psafont psafont commented Feb 12, 2025

I don't know whether breaking behaviour is acceptable or not, opening to receive feedback.

I can always drop the commit with the breaking change and keep the one documenting current behaviour, suggestions on how to deal with this is welcome.

Fixes #418

Current behaviour is unintuitive, let users know how to use the
function, and test it
Change the user-defined function to better match the expectation to
"match" the expected exception.

Fixes mirage#418
@psafont
Copy link
Contributor Author

psafont commented Feb 12, 2025

The failures are:

  • in windows is because the native npm can't be found
  • In the lower bounds check because of a type mismatch (unrelated code to this change)
  • core_unix 0.17 not compiling with ocaml 5.3.0 (and 0.16 with 4.14 under alpine)

Copy link
Collaborator

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

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

I'm in favor of introducing a breaking change to fix the API. Users can set the lower-bound.

Copy link
Member

@tmcgilchrist tmcgilchrist left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for the fix @psafont

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.

Alcotest.match_raises has unintuitive behaviour
3 participants