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

OCaml 5.3 support #489

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Conversation

anmonteiro
Copy link
Contributor

No description provided.

@pmetzger
Copy link
Member

pmetzger commented Sep 3, 2024

Looks at worst harmless in that it only impacts 5.3 and after. However, I think as part of this we should extend the CI to 5.3 since the CI didn't actually check that.

@@ -67,7 +67,28 @@ let rec is_persistent_path = function
#endif

let invalid_package_error_to_string err =
#if OCAML_VERSION >= (5, 2, 0)
#if OCAML_VERSION >= (5,3,0)
Copy link
Contributor

@kit-ty-kate kit-ty-kate Sep 12, 2024

Choose a reason for hiding this comment

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

this section can be nested in the original one. Only two lines are actually different

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to define inline_code compatibility helper rather than deduplicate the full printer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought only the newest cppo had nesting support, so I avoided doing it. Am I wrong?

src/lib/uTop_main.ml Outdated Show resolved Hide resolved
@anmonteiro
Copy link
Contributor Author

Thanks for the helpful code review. The patch is way more minimal now.

@pmetzger
Copy link
Member

Okay, so what's next?

@anmonteiro
Copy link
Contributor Author

it'd be nice for this to be merged and released so that we could release Reason with support for OCaml 5.3 soon.

@anmonteiro
Copy link
Contributor Author

I'm happy to lend some time if utop needs more maintainers

Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

@Octachron Octachron merged commit b490bac into ocaml-community:master Oct 22, 2024
1 check passed
@anmonteiro anmonteiro deleted the anmonteiro/ocaml-5.3 branch October 28, 2024 00:32
@anmonteiro
Copy link
Contributor Author

thanks for merging. would be nice to put together an opam release of utop compatible with OCaml 5.3

@Octachron
Copy link
Member

I will cut a release after the OCaml 5.3.0~beta1 and the 5.2.1 release (aka at the end of this week or next week).

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.

4 participants