Improve: remove option break-string-literals#932
Improve: remove option break-string-literals#932gpetiot merged 3 commits intoocaml-ppx:masterfrom gpetiot:break-string-literals
Conversation
Julow
left a comment
There was a problem hiding this comment.
I agree newlines-and-wrap is better than newlines and wrap and could have been the default.
But this is a huge breaking change. What do you think of using newlines-and-wrap as default and deprecating the option, to be removed in the release after ?
If we want to reduce the number of options we will need a depreciation mechanism and the feedback from users will help us sanitize the options without breaking their workflow.
|
I deprecated the option but kept the values. I'm sure we could factorize the functions defined in Config_option, but it could be done in another PR. |
Julow
left a comment
There was a problem hiding this comment.
We should print a warning when the option is set on the command line or in an .ocamlformat file. Otherwise users won't notice.
|
I added a warning, and checked it is printed when the option is set with command line, attributes, .ocamlformat files or environment, but not with a preset profile. |
|
FWIW this is a feature we definitely want to keep using. We use -let sum = "<?hh
-function sum($a, $b) {
- return $a + $b;
-}"
+let sum = "<?hh\nfunction sum($a, $b) {\n return $a + $b;\n}" let a_contents =
- "<?hh // strict
-
-type my_shape = shape(
- 'outer' => shape(
- B::KEY => string
- ),
-);
-"
-
-let b_contents = "<?hh // strict
-class B {
- const string KEY = \"KEY\";
-}
-"
+ "<?hh // strict\n\n\
+ type my_shape = shape(\n\
+ \ 'outer' => shape(\n\
+ \ B::KEY => string\n\
+ \ ),\n\
+ );\n"
+
+let b_contents =
+ "<?hh // strict\nclass B {\n const string KEY = \"KEY\";\n}\n" | SMUtils.Server_dormant_out_of_retries ->
Printf.eprintf
- ( "Ran out of retries while waiting for Mercurial to finish rebase. Starting "
- ^^ "the server in the middle of rebase is strongly not recommended and you should "
+ ( "Ran out of retries while waiting for Mercurial to finish rebase. \
+ Starting "
+ ^^ "the server in the middle of rebase is strongly not recommended \
+ and you should "
^^ "first finish the rebase before retrying. If you really "I'd assumed 'deprecated' meant "there's a newer option that does this" but it looks like the intent is "we plan to remove this option"? |
|
Yes we planned to remove this option and not replace it. To explain further, the new policy we would like to enforce is to have less options to maintain, as it is getting more and more difficult to maintain it, and we often encounter weird bugs with specific combinations of some options. One of the goal of ocamlformat is to enforce a unique style (or at least very few number of styles), but it seems like everyone wants their own style to be the unique style, or at least to have an option for it, negating this goal and again, too many options makes it difficult to maintain and improve this tool. This option has been the first victim of this policy, but since this option is not especially difficult to maintain we can revert this one (@Julow what do you think?). |
|
It may make sense to keep the option, with @Wilfred For this you should use let a_contents =
{|<?hh // strict
type my_shape = shape(
'outer' => shape(
B::KEY => string
),
);
|}For your last example, it's because of the margin, you should let the string wrap: | SMUtils.Server_dormant_out_of_retries ->
Printf.eprintf
"Ran out of retries while waiting for Mercurial to finish rebase. \
Starting the server in the middle of rebase is strongly not \
recommended and you should first finish the rebase before \
retrying. If you really " |
I did a quick check on github and it seems most projects currently use the
wrapvalue for the option so they should not see too many changes (I fail to see why someone would want the strings to never wrap).The diff of test_branch looks good: