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

Adds missing functions to *Labels modules #875

Merged
merged 1 commit into from Jan 3, 2017

Conversation

Projects
None yet
7 participants
@little-arhat
Contributor

little-arhat commented Oct 25, 2016

This fix make it possible to use labeled modules as drop-in replacement with
open StdLabels.

Added signatures:

(* arrayLabels.mli *)
val iter2 : f:('a -> 'b -> unit) -> 'a array -> 'b array -> unit
val map2 : f:('a -> 'b -> 'c) -> 'a array -> 'b array -> 'c array

(* bytesLabels.mli *)
val extend : bytes -> left:int -> right:int -> bytes
val blit_string :
val cat : bytes -> bytes -> bytes
val uppercase_ascii : bytes -> bytes
val lowercase_ascii : bytes -> bytes
val capitalize_ascii : bytes -> bytes
val uncapitalize_ascii : bytes -> bytes
val equal: t -> t -> bool

(* listLabels.mli *)
val compare_lengths : 'a list -> 'b list -> int
val compare_length_with : 'a list -> len:int -> int
val cons : 'a -> 'a list -> 'a list

(* moreLabels.Hashtbl *)
val is_randomized : unit -> bool

(* stringLabels.mli *)
val uppercase_ascii : string -> string
val lowercase_ascii : string -> string
val capitalize_ascii : string -> string
val uncapitalize_ascii : string -> string
val equal: t -> t -> bool
val split_on_char: sep:char -> string -> string list
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 25, 2016

Member

In principle I agree that keeping the *Labels module on par is a good thing.

Now the nitpicking details. Would we want to have labels on some of the several-arguments function? Here are some potential ideas (but I'm not saying they should absolutely be applied):

val compare_length_with : 'a list -> len:int -> bool
val split_on_char: sep:char -> string -> string list
Member

gasche commented Oct 25, 2016

In principle I agree that keeping the *Labels module on par is a good thing.

Now the nitpicking details. Would we want to have labels on some of the several-arguments function? Here are some potential ideas (but I'm not saying they should absolutely be applied):

val compare_length_with : 'a list -> len:int -> bool
val split_on_char: sep:char -> string -> string list
@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Oct 26, 2016

Contributor

Thank you very much for these additions.
Keeping the *Labels modules on par is not just a good thing, it is actually a requirement: open StdLabels and open MoreLabels should add labels to some functions, without hiding anything.

I agree with @gasche on the labelling of compare_length_with and split_on_char, which is coherent with the rest of StdLabels.

Contributor

garrigue commented Oct 26, 2016

Thank you very much for these additions.
Keeping the *Labels modules on par is not just a good thing, it is actually a requirement: open StdLabels and open MoreLabels should add labels to some functions, without hiding anything.

I agree with @gasche on the labelling of compare_length_with and split_on_char, which is coherent with the rest of StdLabels.

@little-arhat

This comment has been minimized.

Show comment
Hide comment
@little-arhat

little-arhat Oct 26, 2016

Contributor

@gasche, @garrigue I've added labels to these two functions.

By the way, I couldn't find any tests checking compatibility of labeled and non-labeled modules. We could miss to add new functions again in the future without them. Should we add tests for that? The easiest way I could think of would be comparing .mli files, but maybe there is some clever way to check this with type-checker and include module type?

Contributor

little-arhat commented Oct 26, 2016

@gasche, @garrigue I've added labels to these two functions.

By the way, I couldn't find any tests checking compatibility of labeled and non-labeled modules. We could miss to add new functions again in the future without them. Should we add tests for that? The easiest way I could think of would be comparing .mli files, but maybe there is some clever way to check this with type-checker and include module type?

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Oct 26, 2016

Member

It should be easy with a sed script that creates a .ml file, but is there a more elegant way?

Member

damiendoligez commented Oct 26, 2016

It should be easy with a sed script that creates a .ml file, but is there a more elegant way?

@Drup

This comment has been minimized.

Show comment
Hide comment
@Drup

Drup Oct 26, 2016

Contributor
module L : module type of List = ListLabels

for each module, compiled with -nolabels ?

Contributor

Drup commented Oct 26, 2016

module L : module type of List = ListLabels

for each module, compiled with -nolabels ?

@little-arhat

This comment has been minimized.

Show comment
Hide comment
@little-arhat

little-arhat Oct 26, 2016

Contributor

@Drup great! used your idea to add simple tests.

Contributor

little-arhat commented Oct 26, 2016

@Drup great! used your idea to add simple tests.

include module type of Hashtbl
with type statistics := Indirection.t
end
module H : HS = MoreLabels.Hashtbl

This comment has been minimized.

@Drup

Drup Oct 27, 2016

Contributor

module type of struct include Hashtbl end if you want to preserve type equalities. Not sure if that's going to be enough since the fields are not re-exported in MoreLabels.Hashtbl ... but they probably should be.

@Drup

Drup Oct 27, 2016

Contributor

module type of struct include Hashtbl end if you want to preserve type equalities. Not sure if that's going to be enough since the fields are not re-exported in MoreLabels.Hashtbl ... but they probably should be.

This comment has been minimized.

@little-arhat

little-arhat Oct 27, 2016

Contributor

Tried this, same error ...Their kinds differ..

@little-arhat

little-arhat Oct 27, 2016

Contributor

Tried this, same error ...Their kinds differ..

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 27, 2016

Member

@damiendoligez do we want this in 4.04? I'm ready to release a new version of Batteries with 4.04 compatibility, but if we decide to take this one in I would wait for it first.

Member

gasche commented Oct 27, 2016

@damiendoligez do we want this in 4.04? I'm ready to release a new version of Batteries with 4.04 compatibility, but if we decide to take this one in I would wait for it first.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Oct 27, 2016

Contributor

I'm against changing the stdlib a few hours/days before a release. People are supposed to start adapting their libraries when we issue release candidates, and if we start changing things (which can break code in the wild) after release candidates and just before the release, it will reduce people's incentive to play with release candidates in the future. (Of course, this is a bit rhetorical here, since you are also a maintainer of Batteries.)

Not being able to use the most recent additions of stdlib in the *Labels versions is certainly not a critical bug that cannot wait until the next release (which should be out in about 3 months, right?).

Contributor

alainfrisch commented Oct 27, 2016

I'm against changing the stdlib a few hours/days before a release. People are supposed to start adapting their libraries when we issue release candidates, and if we start changing things (which can break code in the wild) after release candidates and just before the release, it will reduce people's incentive to play with release candidates in the future. (Of course, this is a bit rhetorical here, since you are also a maintainer of Batteries.)

Not being able to use the most recent additions of stdlib in the *Labels versions is certainly not a critical bug that cannot wait until the next release (which should be out in about 3 months, right?).

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 27, 2016

Member

I think that's a fair argument.

Member

gasche commented Oct 27, 2016

I think that's a fair argument.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Oct 28, 2016

Member

which should be out in about 3 months, right?

What?

Member

damiendoligez commented Oct 28, 2016

which should be out in about 3 months, right?

What?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Oct 28, 2016

Contributor

I thought 4.05 should be frozen in January 2017. So it would be a bit more than 3 months for the release, but hopefully not much more (I know, I'm dreaming...).

Contributor

alainfrisch commented Oct 28, 2016

I thought 4.05 should be frozen in January 2017. So it would be a bit more than 3 months for the release, but hopefully not much more (I know, I'm dreaming...).

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Nov 2, 2016

Member

I thought 4.05 should be frozen in January 2017.

February, so it'll be frozen, not out, in 3 months.

Member

damiendoligez commented Nov 2, 2016

I thought 4.05 should be frozen in January 2017.

February, so it'll be frozen, not out, in 3 months.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 2, 2016

Contributor

Please Damien let me dream a bit about the delay between freeze and release going down at some point...

Contributor

alainfrisch commented Nov 2, 2016

Please Damien let me dream a bit about the delay between freeze and release going down at some point...

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 4, 2016

Contributor

Coming back to the topic of this PR. Would it be a good opportunity to change the doc strings in *Labels modules to refer to the real module names? (e.g. refer to ArrayLabels.iter2 as ArrayLabels.iter2, not Labels.iter2)

[EDIT] I see that these modules are supposed to be used through open StdLabels. If we stick to this style, the "deprecated" messages in this PR should refer to Array.foo, not ArrayLabels.foo; and also the doc for split_on_char. I've no strong opinion on which is best, but let's be consistent.

Contributor

alainfrisch commented Nov 4, 2016

Coming back to the topic of this PR. Would it be a good opportunity to change the doc strings in *Labels modules to refer to the real module names? (e.g. refer to ArrayLabels.iter2 as ArrayLabels.iter2, not Labels.iter2)

[EDIT] I see that these modules are supposed to be used through open StdLabels. If we stick to this style, the "deprecated" messages in this PR should refer to Array.foo, not ArrayLabels.foo; and also the doc for split_on_char. I've no strong opinion on which is best, but let's be consistent.

Adds missing functions to *Labels modules
This fix make it possible to use labeled modules as drop-in replacement with
`open StdLabels`.

Added signatures:

```ocaml
(* arrayLabels.mli *)
val iter2 : f:('a -> 'b -> unit) -> 'a array -> 'b array -> unit
val map2 : f:('a -> 'b -> 'c) -> 'a array -> 'b array -> 'c array

(* bytesLabels.mli *)
val extend : bytes -> left:int -> right:int -> bytes
val blit_string :
val cat : bytes -> bytes -> bytes
val uppercase_ascii : bytes -> bytes
val lowercase_ascii : bytes -> bytes
val capitalize_ascii : bytes -> bytes
val uncapitalize_ascii : bytes -> bytes
val equal: t -> t -> bool

(* listLabels.mli *)
val compare_lengths : 'a list -> 'b list -> int
val compare_length_with : 'a list -> len:int -> int
val cons : 'a -> 'a list -> 'a list

(* moreLabels.Hashtbl *)
val is_randomized : unit -> bool

(* stringLabels.mli *)
val uppercase_ascii : string -> string
val lowercase_ascii : string -> string
val capitalize_ascii : string -> string
val uncapitalize_ascii : string -> string
val equal: t -> t -> bool
val split_on_char: sep:char -> string -> string list
```
@little-arhat

This comment has been minimized.

Show comment
Hide comment
@little-arhat

little-arhat Nov 27, 2016

Contributor

As per @alainfrisch suggestions, fixed all docs and deprecation warnings in *Labels.

Contributor

little-arhat commented Nov 27, 2016

As per @alainfrisch suggestions, fixed all docs and deprecation warnings in *Labels.

@mshinwell mshinwell added the stdlib label Dec 9, 2016

@little-arhat

This comment has been minimized.

Show comment
Hide comment
@little-arhat

little-arhat Jan 3, 2017

Contributor

Is everything OK with this patch now, or there are some other suggestions to improve it?
Thanks.

Contributor

little-arhat commented Jan 3, 2017

Is everything OK with this patch now, or there are some other suggestions to improve it?
Thanks.

@alainfrisch alainfrisch merged commit 4da2b30 into ocaml:trunk Jan 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jan 3, 2017

Contributor

Thanks for the reminder and the nice contribution.

Contributor

alainfrisch commented Jan 3, 2017

Thanks for the reminder and the nice contribution.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Adds missing functions to *Labels modules (#875)
This fix makes it possible to use labeled modules as drop-in replacement with
`open StdLabels`.

Added signatures:

```ocaml
(* arrayLabels.mli *)
val iter2 : f:('a -> 'b -> unit) -> 'a array -> 'b array -> unit
val map2 : f:('a -> 'b -> 'c) -> 'a array -> 'b array -> 'c array

(* bytesLabels.mli *)
val extend : bytes -> left:int -> right:int -> bytes
val blit_string :
val cat : bytes -> bytes -> bytes
val uppercase_ascii : bytes -> bytes
val lowercase_ascii : bytes -> bytes
val capitalize_ascii : bytes -> bytes
val uncapitalize_ascii : bytes -> bytes
val equal: t -> t -> bool

(* listLabels.mli *)
val compare_lengths : 'a list -> 'b list -> int
val compare_length_with : 'a list -> len:int -> int
val cons : 'a -> 'a list -> 'a list

(* moreLabels.Hashtbl *)
val is_randomized : unit -> bool

(* stringLabels.mli *)
val uppercase_ascii : string -> string
val lowercase_ascii : string -> string
val capitalize_ascii : string -> string
val uncapitalize_ascii : string -> string
val equal: t -> t -> bool
val split_on_char: sep:char -> string -> string list
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment