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
Format.pp_update_geometry: formatter -> (geometry -> geometry) -> unit #9237
Conversation
(cc @Octachron and any other benevolent reviewer) There are two small refactoring commits in this PR, so it is best reviewed commit-by-commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM: Using the new update api looks functionally equivalent to the existing get/set api.
43eef20
to
5cf7790
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me too. I have some minor nitpicking remarks that can be safely ignored:
|
||
let pp_get_margin state () = state.pp_margin | ||
|
||
let pp_set_full_geometry state {margin; max_indent} = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if full
is the right adjective here: maybe unsafe
? Or an adverb directly
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it as-is: this is not exposed in the public API, and the _full
prefix will become self-evident after we start adding extra record fields, while pp_set_geometry
is left partial due to backward-compatibility constraints.
3894550
to
bc574f5
Compare
This lets users write code that is robust to the addition of new geometry fields. Format.(pp_update_geometry ppf (fun geo -> {geo with ...})) Today the only way to set the geometry is Format.pp_set_geometry ppf ~margin ~max_indent we cannot add optional parameters after the [ppf] argument, and adding new labeled parameters would break user code. (Also: it's often convenient to work with the record directly, for example to reset a previous geometry saved with [pp_get_geometry]; this is indirectly provided by [pp_update_geometry].)
(The CI failure on AppVeyor appears to be due to memprof, and thus unrelated.) |
This lets users write code that is robust to the addition of new
geometry fields.
Today the only way to set the geometry is
we cannot add optional parameters after the [ppf] argument, and adding
new labeled parameters would break user code.
(Also: it's often convenient to work with the record directly, for
example to reset a previous geometry saved with [pp_get_geometry];
this is indirectly provided by [pp_update_geometry].)