Skip to content

Comments

Use ormolu 0.5.0.1 in dev environment#2780

Merged
elland merged 6 commits intodevelopfrom
smatting/ormolu-5
Oct 26, 2022
Merged

Use ormolu 0.5.0.1 in dev environment#2780
elland merged 6 commits intodevelopfrom
smatting/ormolu-5

Conversation

@smatting
Copy link
Contributor

@smatting smatting commented Oct 20, 2022

Motivation: Consistency with HLS, which uses the same version. That way if you use HLS for formatting you get the same result.

Ormolu now has formatting depending on the fixity of of operators, which it tries to parse out of files. Sometimes this approach fails that's why you can introduce overrides for fixities in .ormolu files which need to be next to .cabal files.

Without these overrides our reformatting is really ugly

infixr 10 .=
infix 4 ===
infix 4 =/=
infixr 3 !!!
infixr 3 <!!

These definitions are the same as in Bilge.Assert

infix 4 ===
infix 4 =/=
infixr 3 !!!
infixr 3 <!!

somehow ormolu was not able to read these fixities out of the module.

I don't know why we need this (it's not the same fixity as aeson defines it):

infixr 10 .=

I would consider it a workaround.

Checklist

@smatting smatting temporarily deployed to cachix October 20, 2022 09:49 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 20, 2022
@smatting smatting temporarily deployed to cachix October 20, 2022 10:08 Inactive
@smatting smatting temporarily deployed to cachix October 20, 2022 11:10 Inactive
@smatting
Copy link
Contributor Author

We don't like these changes, look how it breaks the lines with the (.=) operator (both schema-profunctor and aeson)

@smatting
Copy link
Contributor Author

That result is really ugly. I think we might suffering from the same issue as here tweag/ormolu#892
The issuer suggests you can override fixities, I think you can specify them in a .ormolu file https://hackage.haskell.org/package/ormolu cc @akshaymankar

@akshaymankar
Copy link
Member

I am disappointed that there is a .ormolu file despite saying

Implementing one “true” formatting style which admits no configuration.

But, let's try it out anyway, hopefully people won't have diverging opinions on this 🤞

@smatting smatting temporarily deployed to cachix October 20, 2022 14:12 Inactive
@smatting smatting marked this pull request as ready for review October 20, 2022 14:18
@smatting
Copy link
Contributor Author

smatting commented Oct 20, 2022

I've added the necessary overrides to .ormolu files. I've added a lot of symlinks, because you need to have a .ormolu file per cabal project. I updated the PR description with some details.

Comment on lines 89 to +90
:- Capture "id" (GroupId tag)
:> Get '[SCIM] (StoredGroup tag),
:> Get '[SCIM] (StoredGroup tag),
Copy link
Member

Choose a reason for hiding this comment

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

Can this also be fixed with the .ormolu file?

Comment on lines -203 to +206
AssetToken <$> assetTokenAscii
.= schema
& doc' . S.schema . S.example ?~ toJSON ("aGVsbG8" :: Text)
AssetToken
<$> assetTokenAscii
.= schema
& doc' . S.schema . S.example ?~ toJSON ("aGVsbG8" :: Text)
Copy link
Member

Choose a reason for hiding this comment

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

Is this right? I am actually not sure if doc' is applied to schema or (AssetToken <$> assetTokenAcii .= schema). From the original code it looks like it is applied to schema, now it looks like that later.

Copy link
Contributor

Choose a reason for hiding this comment

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

& is infixl 1 so it is applying to AssetToken <$> assetTokenAscii .= schema

accept "application" "json"
.&> query "key"
.&. query "code"
.&. query "code"
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong, maybe .&> and .&. should be at the same fixity?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently both are infixr 3, though it could be possible that Ormolu isn't picking that up? Prehaps we need to add them to the .ormolu file?

I don't understand why Ormolu wants to indent these lines. I can't come up with a logic that is consistent with how it handlles (: and ++) or (. and $) in other places.

That said, I really don't think "ugly" formatting should be a blocker for this task. If we need to change formatter, or add ORMOLU_DISABLE sections, or quiet hlint, or other we can take that up as an effort separate from aligning CI Ormolu with dev environment HLS/Ormolu.

@stephen-smith
Copy link
Contributor

infixr 10 .=

it's not the same fixity as aeson

Data.Schema is our definition of this. That file doesn't have a fixity declaration, which might be why Ormolu can't find it. We should probably add one, either at 4 (to match lens) or 8 (to match aeson). The "default" is infixl 9 (per the Haskell Report) but I don't know if that's how Ormolu is treating it.

Copy link
Contributor

@stephen-smith stephen-smith left a comment

Choose a reason for hiding this comment

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

Changes are "fine".

New newer Ormolu adds more line breaks and indentation than I really like.

@elland elland temporarily deployed to cachix October 26, 2022 09:39 Inactive
@elland elland merged commit e03f721 into develop Oct 26, 2022
@elland elland deleted the smatting/ormolu-5 branch October 26, 2022 11:23
@flokli
Copy link
Contributor

flokli commented Nov 7, 2022

This accidentially reverted the submodule update in services/nginz/third_party/nginx-module-vts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants