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

Add equality between Stdlib.result and BatInnerPervasives.result #939

Closed
wants to merge 1 commit into from

Conversation

clembu
Copy link

@clembu clembu commented Jan 16, 2020

See #938

I agreed with the point raised by the issue author. I don't think more changes are needed. If I missed some places to put tests, I'd like to know.

Copy link
Contributor

@rixed rixed left a comment

Choose a reason for hiding this comment

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

I we do this, then batteries will become incompatible with earlier versions of the compiler.
The right way to work around this is to create the type for compiler which version is lesser than the one where the result type appeared in the Stdlib, and to equate the types after that point.
That is why we have a source preprocessor in batteries (the *.mlv and *.mliv files).

So in this case, the change should be prepended with ##V>=X## and the alternative with ##V<X##.

@rixed
Copy link
Contributor

rixed commented Jan 16, 2020

According to OCaml "Changes" file, the result type appeared in OCaml v4.08.

@clembu
Copy link
Author

clembu commented Jan 16, 2020

okay, I see. Makes sense. Thanks, I'll do that.

@UnixJunkie
Copy link
Member

A possible test is to opam switch to 4.07 and compile batteries, then switch to 4.08 and recompile.
I am not sure if it is possible to unit-test this.

@hcarty
Copy link
Contributor

hcarty commented Jan 17, 2020

The result type appeared in 4.03. A Result module was added in 4.08.

@clembu
Copy link
Author

clembu commented Jan 17, 2020

so, no need to change the type definition?
Though I may have to remove the compatibility with Legacy.Result, right? because before 4.07 that module doesn't exist?

@UnixJunkie
Copy link
Member

For things which are version-dependant, you can use preprocessor directives, as stated before by rixed.

@clembu
Copy link
Author

clembu commented Jan 17, 2020

As a matter of fact, the compatibility with the Stdlib.Result module fails because Stdlib.Seq.t is not BatSeq.t. I'll look into that too.

@rixed rixed mentioned this pull request Apr 13, 2020
@rixed
Copy link
Contributor

rixed commented Apr 13, 2020

Superseeded by #957

@rixed rixed closed this Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants