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

Jordan: Force breaks after labels if child requires so #10

Merged
merged 2 commits into from
Dec 7, 2015

Conversation

mjambon
Copy link
Member

@mjambon mjambon commented Dec 7, 2015

This generalizes the Force_breaks_rec feature to labels. I introduced a new label parameter called label_break which defines whether the break after the label is automatic (normal behavior), or should be forced.

Just like for lists, we can also force a break after a label and require a break in all the containing lists or labels.

The example posted in #8 now looks good (first case for the label with inner list using Force_breaks_rec`):

*** label with inner list using `Force_breaks_rec ***
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
reallyLongLabelOne
  reallyLongLabelTwo
    reallyLongLabelABC [
      1.23456,
      9.87654,
      9.87654,
      9.87654
    ]

I didn't write tests or examples for the new Always_rec` or Never, but we should. I don't know if ``Never will ever be useful, I added it primarily for completeness.

that uses Force_breaks_rec or a child label that uses Always_rec.
space_after_label : bool;
indent_after_label : int;
label_style : style_name option;
}

let label = {
label_break = `Auto;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm realizing that having functions with named args/defaults be the primary way to construct them - might allow evolving of the API in a backwards compatible way. Just a realization - not important.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally users would only use the record update syntax, e.g. { default_from_library with label_break =Always }`, which tolerates the addition of future fields. Providing functions with optional arguments makes it a little more obvious I guess but doesn't let the user inherit and override some fields from other sources than the default:

(* provided by library *)
val create_config : ?a:a -> ?b:b -> unit -> config

(* user code *)
let my_default_config =  create_config ~a:`Whatever ()
let special_config = ???

In order to create special_config, currently we'd use the with syntax. If we only provided labeled functions, we'd need a higher-order function that does the same. It would have the following type:

(* library would provide this *)
val config_class : ?a:a -> ?b:b -> unit -> (?a:a -> ?b:b -> unit -> config)

(* user code *)
let create_config = config_class ~a:`Whatever ()
let my_special_config = create_config ~b:`Bananas ()

The user code will remain compatible when future options are added as long as create_config is left without a type annotation that would lock the number and name of the arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh and if we want to inherit in an infinite chain, we need to provide something like this:

val inherit_config : ?a:a -> ?b:b -> (?a:a -> ?b:b -> unit -> config) -> (?a:a -> ?b:b -> unit -> config)

Copy link
Member Author

Choose a reason for hiding this comment

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

Real OOP might be more appropriate for this.

@jordwalke
Copy link
Collaborator

Looks great! Names seem fine, too.

@jordwalke
Copy link
Collaborator

Great points! Want to merge?

mjambon added a commit that referenced this pull request Dec 7, 2015
Force breaks after labels if child requires so
@mjambon mjambon merged commit 6e0b27b into master Dec 7, 2015
@mjambon mjambon deleted the mj/break-labels branch December 7, 2015 21:56
@mjambon
Copy link
Member Author

mjambon commented Dec 7, 2015

Preparing a release.

@mjambon
Copy link
Member Author

mjambon commented Dec 7, 2015

opam release status: ocaml/opam-repository#5264

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.

2 participants