Chore: formatting, explicit imports. #5847
Conversation
Unisay
force-pushed
the
yura/term-eval-order-builtin-arity
branch
from
2092385
to
17c0a19
Compare
| import Data.DList qualified as DList | ||
| import Data.List.NonEmpty qualified as NE | ||
| import PlutusCore.Builtin (BuiltinMeaning (..), ToBuiltinMeaning (..), TypeScheme (..)) |
There was a problem hiding this comment.
I don't think we reached any agreement on whether this is desirable or not. For me, this makes things worse, since these are project modules that I would expect people to be familiar with.
There was a problem hiding this comment.
these are project modules that I would expect people to be familiar with.
Actually, in my case it is other way around: I might be acquainted with the Prettyprinter symbols from using the library in other projects, but I don't yet know all the symbols exported by our PlutusTx modules. 🤷🏼♂️
Anyway, I am not particularly hang up on explicit imports, I can undo those ones you prefer to leave wildcard.
There was a problem hiding this comment.
We don't have a policy. So I'm not saying don't do it - we don't have a policy so we simply have various preferences, and yours are just as valid.
But I would much prefer to have a policy.
There was a problem hiding this comment.
I agree that we should have a policy. I personally prefer explicit export lists, and I think this can be enforced somewhere either in the formatter (once we decide what we want) or in the commit pipeline. Maybe we should talk about this during the technical discussion?
| -- or we don't know what comes next. | ||
| data EvalTerm tyname name uni fun a = | ||
| Unknown | ||
| {- | Either the "next" term to be evaluated, along with its 'Purity' and 'WorkFreedom', |
There was a problem hiding this comment.
The more this happens the more I dislike our current approach of "people can just reformat things to their personal preference". It's terrible. Please can we decide to either use fourmolu consistently or just not reformat things.
There was a problem hiding this comment.
At the moment nothing stops someone just reformatting this back a different way with "Chore: reformatting" :(
There was a problem hiding this comment.
Only a few things here are decided to match my personal preference (e.g. explicit imports and some naming choices, etc.)
The majority of formatting decisions falls in one of 3 categories here:
- Developer's personal preference.
- A thing that is advised by the STYLEGUIDE (e.g. 2 spaces, not 4)
- A thing that is decided by a
fourmoluand isn't configurable. - A thing that is decided by a shared project-specific
fourmoluconfiguration.
My goal is to have most of the things fall into 1 or 3.
I am willing to undo the changes you think are 0 or 2.
There was a problem hiding this comment.
For plutus, my formatter is set to stylish-haskell and it seems to format new code in the same way the existing code in the module is formatted (which can be different from module to module). I have no idea how that happens. I use VSCode if that makes any difference.
Anyway, I agree that it's weird that formatting keeps changing on a module to module basis. @michaelpj did the team try to use fourmolu at some point? What didn't they like about it?
There was a problem hiding this comment.
stylish-haskell and it seems to format new code in the same way the existing code in the module is formatted
Two reasons:
- we already enforce stylish-haskell (git hooks) .
- stylish-haskell is very liberal: it does change the imports and aligns vertical arrows, equals, etc. It won't change indentation.
By the way, I have configured local formatting such that fourmolu -produced output is piped to the stylish-haskell: their combination leads to a least change.
There was a problem hiding this comment.
A thing that is decided by a fourmolu
Nothing in our style guide or contributing guide even says that we use fourmolu at all.
By the way, I have configured local formatting such that fourmolu -produced output is piped to the stylish-haskell: their combination leads to a least change.
😱
did the team try to use fourmolu at some point? What didn't they like about it?
@effectfully is the main opponent, you can search in slack for some discussion.
There was a problem hiding this comment.
@ana-pantilie see this for example.
There was a problem hiding this comment.
see #5761 (review) for example.
I initially read the first diff as fourmolu removing the INLINEABLE pragmas which got me very scared, but I see it's only adding a newline between the pragma and the type declaration, right? I looked up the docs and INLINEABLE or INLINE can be added anywhere in the module I believe, https://downloads.haskell.org/ghc/latest/docs/users_guide/exts/pragmas.html#inlinable-pragma.
The other changes you pointed out aren't really dangerous, are they? If they're not, although I agree that they're ugly, I think they're not that bad once you consider the bigger picture. Don't hate me, but I think that as long as these "formatting bugs" are cosmetic and relatively infrequent it's a small price to pay for having consistent automated formatting.
Let me undo those decisions that make you unhappy. What is it in particular? |
|
"unhappy" was meant to attach to "ad-hoc" rather than the specific decisions :) I just think it's past time we agreed on a policy. |
Housekeeping chore: format a few modules (
Purity) before changing them.