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 check-typo compatibility to recent GPRs #1874

Merged
merged 4 commits into from Jul 1, 2018

Conversation

Projects
None yet
3 participants
@dra27
Copy link
Contributor

commented Jul 1, 2018

Since #1288 was merged #1061, #1833, #1844 and #1845 have been merged to trunk without check-typo being run on them.

I intentionally merged #1148 in order to test the trunk checking on Inria's CI which was lucky because it didn't work and required bc3dedd to ensure that the job failed as it's supposed to!

This GPR should restore the CI to blue/green.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2018

@@ -166,7 +166,8 @@ type expression =
| Cifthenelse of expression * expression * expression
| Cswitch of expression * int array * expression array * Debuginfo.t
| Cloop of expression
| Ccatch of rec_flag * (int * (Ident.t * machtype) list * expression) list * expression
| Ccatch of rec_flag * (int * (Ident.t * machtype) list * expression) list *
expression

This comment has been minimized.

Copy link
@gasche

gasche Jul 1, 2018

Member

When you break line in the middle variant constructor descriptions, it is nicer to have the star symbol at the beginning of the next line.

This is my personal taste (in general with infix operators) but also a style that is used in the compiler:

$ git grep -B2 "^ *\*[^)][^o]" typing/*.mli | cat
typing/includemod.mli-  | Value_descriptions of Ident.t * value_description * value_description
typing/includemod.mli-  | Type_declarations of Ident.t * type_declaration
typing/includemod.mli:        * type_declaration * Includecore.type_mismatch list
--
typing/typedtree.mli-  | Tcl_fun of
typing/typedtree.mli-      arg_label * pattern * (Ident.t * expression) list
typing/typedtree.mli:      * class_expr * partial

This comment has been minimized.

Copy link
@dra27

dra27 Jul 1, 2018

Author Contributor

Ah, OK - I thought I found conflicting examples (or maybe I couldn't, and just went along with arrows, which I think are often left dangling).

This comment has been minimized.

Copy link
@gasche

gasche Jul 1, 2018

Member

Hm, looking for it (git grep -B2 "[^(]\* *$" *.mli) there are examples of using the opposite style all over, some of which written by myself... Then there is this nice grep result:

typing/typedtree.mli      * class_expr * partial
typing/typedtree.mli  | Tcl_apply of class_expr * (arg_label * expression option) list
typing/typedtree.mli  | Tcl_let of rec_flag * value_binding list *

In other words: do as you please.

This comment has been minimized.

Copy link
@dra27

dra27 Jul 1, 2018

Author Contributor

I suppose another option for these particular long tuple types might be to go down the same road as for records:

 | Ccatch of rec_flag
           * (int * (Ident.t * machtype) list * expression) list
           * expression

This comment has been minimized.

Copy link
@dra27

dra27 Jul 1, 2018

Author Contributor

(although aligning the asterisks with the rec_flag is probably better ... and possibly even admitting a redundant first * as we do for |?!?!)

@dra27 dra27 force-pushed the dra27:check-typo-fixes branch from 7ac7ad9 to fe781a5 Jul 1, 2018

@dra27 dra27 merged commit dc221ff into ocaml:trunk Jul 1, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2018

Marvellous - CI back in the blue...

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2018

Sorry @shindere - that's part of the reason I did it on a Sunday! Too many of us keenly hacking at the weekend.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.