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

Change Ojs_exn to Ojs.Exn #121

Open
rgrinberg opened this issue Nov 1, 2020 · 2 comments
Open

Change Ojs_exn to Ojs.Exn #121

rgrinberg opened this issue Nov 1, 2020 · 2 comments

Comments

@rgrinberg
Copy link
Contributor

It's recommended that every library has a single toplevel module, and (wrapped false) is only there to help people transition to dune. Would it be ok to break compatibility and to switch to Ojs.Exn? If not, then perhaps we could add something to dune to help the transition from (wrapped false) to (wrapped true). We already have one such mechanism (wrapped (transition "message")) but it doesn't quite work in some cases (like this one)

@hhugo
Copy link
Contributor

hhugo commented Nov 1, 2020

Why doesn't the "transition" mechanism work in that case ?

@rgrinberg
Copy link
Contributor Author

In this case, Ojs_exn depends on Ojs. So we'd need to extract Ojs.t to some other module core, and then write ojs.ml as:

include Core
module Exn = Exn

But that would mean that (wrapped (transition ..)) would create a deprecated alias of the form ojs_core. This seems unnecessary.

I suppose that in this case we can just inline ojs_exn into ojs and then write ojs_exn as:

include Ojs.Exn

With a deprecation message. That seems alright to me.

@mlasson mlasson linked a pull request Feb 1, 2021 that will close this issue
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 a pull request may close this issue.

2 participants