Check formatting in CI #29
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This required removing some of the "Jane Street extension" markers, as ocamlformat was moving them to unhelpful places. Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
ccasin
approved these changes
Jul 24, 2023
ccasin
left a comment
There was a problem hiding this comment.
LGTM, but I'll wait for @goldfirere to weigh in before merging because I know he is fond of the comments.
|
Groan. I actually don't care all that much about the comments. But I do really like the blank lines -- I think separating out our changes with blank lines will make merge conflicts less likely (but I have no evidence for this). Why did the comments disappear? ocamlformat shouldn't eat them. Did they get moved around to the point that they weren't useful, so you got rid of them? (That's reasonable -- but then we lose the blank lines, which is really what I care about.) |
Author
|
That’s right, but even more so – ocamlformat ate the blank lines and did things like put the comments at the ends of preceding lines. That’s the tradeoff with auto-formatting – there’s no formatting noise, but you can’t use formatting to send a signal. |
|
Well, what do we think here? I lean slightly against merging this, but if you both want to, I won't stand in the way. |
I agree there's some chance the whitespace reduces merge conflicts, or at least makes it clearer when they are just caused by changes on adjacent lines. And perhaps helps reading the code. But, to balance against that is the fact that formatting ocamlformat itself is the only large test of real code in the ocamlformat testsuite. Certainly, I have found bugs this way in the past. I think it would be much sadder to lose this than to lose the new comments. So, I vote merge. |
|
Chris makes a good argument: evidence of bug-finding vs my hypothesis about merge conflicts. Let's go with fewer bugs. Merging. |
ccasin
pushed a commit
that referenced
this pull request
Sep 19, 2023
* Fix formatting This required removing some of the "Jane Street extension" markers, as ocamlformat was moving them to unhelpful places. Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> * Check formatting in CI Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> --------- Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
mdelvecchio-jsc
pushed a commit
that referenced
this pull request
Sep 20, 2023
* Fix formatting This required removing some of the "Jane Street extension" markers, as ocamlformat was moving them to unhelpful places. Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> * Check formatting in CI Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> --------- Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
mdelvecchio-jsc
pushed a commit
that referenced
this pull request
Oct 12, 2023
(Note for future rebases: at this stage it is expected that `dune b
@test/passing/runtest` will give errors related to extra
`[@ocaml.curry]`s)
local: support local_ and curry in function types
Note for future rebases: it is expected that `dune b
@test/passing/runtest` will fail at this stage, in my case with
output:
File "test/passing/dune.inc", line 3594, characters 0-205:
3594 | (rule
3595 | (deps tests/.ocamlformat )
3596 | (package ocamlformat)
3597 | (action
3598 | (with-stdout-to local.ml.stdout
3599 | (with-stderr-to local.ml.stderr
3600 | (run %{bin:ocamlformat} --margin-check %{dep:tests/local.ml})))))
Command exited with code 1.
local: support 'fun (local_ x) ... -> '
local: nonlocal/global record field decls
For future reference during rebasing: `dune b @test/passing/runtest`
is not expected to pass on this commit. Some uses of `local_` are
turned into `[%extension.local]`.
local: return expressions 'fun ... -> local_ ...'
For reference during future rebases: `dune b @test/passing/runtest` is
expected to pass on this commit.
local: apply formatting
For future rebases: `make test` should pass at this stage. It may be
necessary to run `dune fmt` and `make regtests-promote` (but I checked
`dune b @test/passing/runtest` succeeded first)
Update to ocaml-jst a78975eb57
Autogenerated attributes are now 'extension.foo' instead of 'ocaml.foo'
change module types to export equalities
Signed-off-by: Cameron Wong <cwong@janestreet.com>
Support for "include functor" (#5)
* Initial support for the include_functor extension
* Add test for include functor
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
local: bugfix for punned named arguments with type annotations (#3)
A bug in the previous patch for locals causes ocamlformat to incorrectly
format local arguments which are punned with a label and have a type signature:
fun ~(local_ x : t) -> ...
by putting an extra invalid pair of parens around (x : t). This patch removes
the parens.
Signed-off-by: Stephen Dolan <sdolan@janestreet.com>
Ensure parentheses are placed around (local_ E) when necessary (#4)
Signed-off-by: Stephen Dolan <sdolan@janestreet.com>
Remove unnecessary parentheses around expressions with trailing [@attrs] (#6)
Signed-off-by: Stephen Dolan <sdolan@janestreet.com>
update parsers and printer for polymorphic parameters (with tests)
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Support for comprehensions and immutable arrays
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Update to the improved language extension infrastructure
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
add support for global constructor arguments
Signed-off-by: Zesen Qian <github@riaqn.org>
support for layout annotations on type parameters
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Address review feedback.
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Fix a test failure due to misformatted comment
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Fix a (CI) test failure in help printing due to version mismatch
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
support exclave_ syntax
Signed-off-by: Zesen Qian <github@riaqn.org>
no fail fast (#24)
Signed-off-by: Zesen Qian <github@riaqn.org>
Don't desugar `let _ = local_ x` (#21)
* Don't desugar `let _ = local_ x`
The desugared syntax `let local_ _ = x` is invalid.
Signed-off-by: Jules Aguillon <jules@j3s.fr>
* Restrict `let local_ ...` rewrite even more
This restricts the desugaring of `let x = local_ y` to the cases where
`x` is an identifier or an identifier with a type annotation.
Signed-off-by: Jules Aguillon <jules@j3s.fr>
* Tests: Add a .ref file for local.ml
There are syntax rewrite on locals, it's important not to change the
test input.
* Fix local_ desugaring generating invalid syntax
These two items don't have the same AST and must be recognized:
let local_ x : string = "hi"
let (x : string) = local_ "hi"
* Preserve `local_` position when ptyp_poly constraint
These two items don't have the same AST:
let x : 'a . 'a -> 'a = local_ "hi"
let local_ x : 'a. 'a -> 'a = "hi"
---------
Signed-off-by: Jules Aguillon <jules@j3s.fr>
better exclave_ formatting (#28)
Signed-off-by: Zesen Qian <github@riaqn.org>
Support unboxed float literals (#25)
* Automatic changes to patch files
This just happened when I hit `dune build`. Don't really know what's
going on.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Updated the standard parsetree for unboxed literals
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Support unboxed literals
Testing story not complete yet; want advice.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Make parsetree changes separated
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Checkpoint
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Still not quite working, but done for tonight
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Vendor in jane-syntax into standard
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Separate out Jane Street stuff more
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* More tests, and a bugfix
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
---------
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Add instructions for how to update ocamlformat (#26)
* Write instructions
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Chris's comments
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Document testing
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Typo
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Follow on from Chris's edits
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Further advice about dune errors
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Comment about `make test`
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
---------
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Co-authored-by: Chris Casinghino <ccasinghino@janestreet.com>
Support float# (#27)
* Update parser-standard for float#
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Add support for float#
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Add tests
But this doesn't work. I'm very confused about how to test here.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Some more type tests
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Test float#
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Forward-proof printing of float#
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Update parser-standard/language_extension
This also removes the differences between ocamlformat's
language_extension and janestreet's. Instead, we now just
call enable_maximal. Seems simpler.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
---------
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Check formatting in CI (#29)
* Fix formatting
This required removing some of the "Jane Street extension" markers, as
ocamlformat was moving them to unhelpful places.
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Check formatting in CI
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
---------
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Capture a few notes from our working conventions (#30)
* Capture a few notes from Working Conventions
Merging this will allow me to stub out the section in Working
Conventions.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Chris's suggestions
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
---------
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Add a mode for erasing erasable jane-syntax (#32)
* Add (untested) support for erasing erasable Jane syntax
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Make normalization ignore erasable jane syntax if requested
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Add failing test of erasure
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Update parser-standard to antalsz/flambda-backend 5b23940cfccd3047
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Update parser-standard and location to antalsz/flambda-backend c056f5f2b0082711b596337f270a8eccf06d0b06
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Update `--erase-jane-syntax` documentation per review
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Documentation for `Erase_jane_syntax`
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Only remove erasable `Jane_syntax` attributes from the old AST
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Fix formatting
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
---------
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Add support for float64 layout (#35)
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Support explicit strengthening (#39)
* Support explicit strengthening
Support the syntax introduced in
oxcaml/oxcaml#1337.
Signed-off-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
* Fix parens with attributes
Signed-off-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
---------
Signed-off-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
Co-authored-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
Update build workflow to OCaml 4.14 (#33)
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
In an if-then with no else, the body should have parens if local_ (#40)
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Add a mode for rewriting `[@Local]`, `[%local]`, `[@ocaml.global]`, etc. to their keyword forms (`local_`, `global_`, etc.) (#38)
* Add mode to rewrite `[@Local]`, etc., into `local_`
This is available under the option
`--rewrite-old-style-jane-street-local-annotations`
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Add tests of rewriting `[@Local]` and fix the normalization check
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Update `ocamlformat-help.txt`
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Remove export of function that was for debugging
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
---------
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Fix bug caused by comments before local_ at the beginning of bindings. (#41)
Signed-off-by: Thomas Del Vecchio <tdelvecchio@janestreet.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We didn't run ocamlformat's self-formatting check in CI; now we do.
I deleted many of the "Jane Street extension" markers, as the auto-formatting was putting them in strange places.