Support (layouting) config inline/pragma-style #30

New Issue

2017-05-25T21:09:21+02:00

lspitzner commented

2017-05-25 21:09:21 +02:00

(Migrated from github.com)

In some cases I noticed that per-file config is desirable, or even more fine-grained control ("leave this function/block alone" etc.)

This is mildly easy to implement, but I could use feedback regarding the interface design: What kind of pragma to use. I see the following options:

Use real pragma
```
{-# OPTIONS_BRITTANY --columns=80 #-}
```
This pretends to be a real pragma, and ghc will promptly warn about it being unknown.
Can be disabled, but its annoying and I dislike this option.

Note that haddock uses these, due to haddock being wired into ghc, as I understand
it.
Use fake pragma
```
{- # OPTIONS_BRITTANY --columns=80 # -}
```
As I have gathered from the archives Haddock used this option earlier.
Use ANN feature
```
module .. where
{-# ANN module "Brittany: --columns=80" #-}
{-# ANN myFunction "Brittany: ignore" #-}
```
This is a "proper" approach to annotating the whole module or single functions.
While this option is closer to "standard" syntax for the purpose, it has some annoying
downside: The above pragma can not be placed as freely as one might think (and the
errors you get are rather confusing). For example, you cannot move it above the header
("module Foo where"), below any LANGUAGE pragmas.

Use something based on raw comments, like

{- BRITTANY --columns=80 -}
-- BRITTANY --columns=80

Opinions/Arguments?

In some cases I noticed that per-file config is desirable, or even more fine-grained control ("leave this function/block alone" etc.) This is mildly easy to implement, but I could use feedback regarding the interface design: What kind of pragma to use. I see the following options: 1. Use real pragma ~~~~.hs {-# OPTIONS_BRITTANY --columns=80 #-} ~~~~ This pretends to be a real pragma, and ghc will promptly warn about it being unknown. Can be disabled, but its annoying and I dislike this option. Note that haddock uses these, due to haddock being wired into ghc, as I understand it. 2. Use fake pragma ~~~~.hs {- # OPTIONS_BRITTANY --columns=80 # -} ~~~~ As I have gathered from the archives Haddock used this option earlier. 3. Use ANN feature ~~~~.hs module .. where {-# ANN module "Brittany: --columns=80" #-} {-# ANN myFunction "Brittany: ignore" #-} ~~~~ This is a "proper" approach to annotating the whole module or single functions. While this option is closer to "standard" syntax for the purpose, it has some annoying downside: The above pragma can not be placed as freely as one might think (and the errors you get are rather confusing). For example, you cannot move it above the header ("module Foo where"), below any `LANGUAGE` pragmas. 4. Use something based on raw comments, like ~~~~.hs {- BRITTANY --columns=80 -} -- BRITTANY --columns=80 ~~~~ ---- Opinions/Arguments?

ElvishJerricco commented

2017-05-25 21:27:24 +02:00

(Migrated from github.com)

This is an awesome idea.

tfausak commented

2017-05-25 22:31:22 +02:00

(Migrated from github.com)

I would prefer the fourth option with normal comments. It matches what I'm used to from other languages. For example, Ruby's RuboCop uses comments for this: http://rubocop.readthedocs.io/en/latest/configuration/#disabling-cops-within-source-code

ElvishJerricco commented

2017-05-25 22:46:13 +02:00

(Migrated from github.com)

Yea I think the choices are between options 3 and 4 for me. I'd lean toward option 3, but I'd have to know more about its limitations on placement, and what exactly it would look like in practice

eborden commented

2017-09-21 23:02:26 +02:00

(Migrated from github.com)

I like option 4. I've used annotations and they are powerful, but they are also very noisy.

lspitzner commented

2017-10-01 17:50:55 +02:00

(Migrated from github.com)

I went with 4 for now, mostly due to laziness on my part, and because it is not possible to express everything we may want with ANNs anyway: There is no way to annotate instances, while -- BRITTANY @ vvv --flag=value will work for those - even when we don't support instances yet.

If you mess up the syntax after a -- BRITTANY comment brittany will error out. Perhaps a warning is more appropriate.

Implementing the per-binding version will require a slightly different implementation or changes to butcher to make it parse an arbitrary "command".

I went with 4 for now, mostly due to laziness on my part, and because it is not possible to express everything we may want with ANNs anyway: There is no way to annotate instances, while `-- BRITTANY @ vvv --flag=value` will work for those - even when we don't support instances yet. If you mess up the syntax after a `-- BRITTANY` comment `brittany` will error out. Perhaps a warning is more appropriate. Implementing the per-binding version will require a slightly different implementation or changes to `butcher` to make it parse an arbitrary "command".

👍 1

lspitzner commented

2017-10-01 19:04:14 +02:00

(Migrated from github.com)

Btw the exact syntax (i.e. "@" and "vvv") are of course still open for discussion.

eborden commented

2017-10-02 18:02:03 +02:00

(Migrated from github.com)

@lspitzner So this will enable easier testing via tweaking individual layout options per test?

lspitzner commented

2017-10-02 18:14:33 +02:00

(Migrated from github.com)

yes. e.g. we now can add

#test fourindent-1

-- BRITTANY @ vvv --indent=4
func = if x
    then if y -- y is important
        then foo
        else bar
    else Nothing

to tests.blt and it succeeds.

On the one hand having to specify the config per-testcase might become redundant, but the upside is that all tests remain self-contained.

Should probably add support for more than one *.blt file though; 133 cases in one file is becoming unwieldy.

yes. e.g. we now can add ~~~~ #test fourindent-1 -- BRITTANY @ vvv --indent=4 func = if x then if y -- y is important then foo else bar else Nothing ~~~~ to `tests.blt` and it succeeds. On the one hand having to specify the config per-testcase might become redundant, but the upside is that all tests remain self-contained. Should probably add support for more than one `*.blt` file though; 133 cases in one file is becoming unwieldy.

tfausak commented

2017-10-02 18:35:15 +02:00

(Migrated from github.com)

Bike shedding syntax for a second: I'm not crazy about -- BRITTANY @ vvv ... for annotating the next declaration. ESLint has a lot of ways to control it through comments: https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments

eslint-disable / eslint-enable: Globally disables/enables ESLint. Similar to a block comment. Useful for ignoring entire sections of code.
eslint-disable-line: Disables ESLint on the current line. Useful for ignoring one or two things on short lines.
eslint-disable-next-line: Disables ESLint on the next line. Useful for ignoring lots of stuff on long lines.

Each comment also takes a list of options for disabling particular linters, like eslint-disable foo, bar.

Translating that to Brittany, I would like to see something like: -- brittany-next-declaration --indent=4. That way it's crystal clear just from reading the comment what's going on.

Bike shedding syntax for a second: I'm not crazy about `-- BRITTANY @ vvv ...` for annotating the next declaration. ESLint has a lot of ways to control it through comments: https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments - `eslint-disable` / `eslint-enable`: Globally disables/enables ESLint. Similar to a block comment. Useful for ignoring entire sections of code. - `eslint-disable-line`: Disables ESLint on the current line. Useful for ignoring one or two things on short lines. - `eslint-disable-next-line`: Disables ESLint on the next line. Useful for ignoring lots of stuff on long lines. Each comment also takes a list of options for disabling particular linters, like `eslint-disable foo, bar`. Translating that to Brittany, I would like to see something like: `-- brittany-next-declaration --indent=4`. That way it's crystal clear just from reading the comment what's going on.

👍 2

lspitzner commented

2017-10-02 20:46:41 +02:00

(Migrated from github.com)

I am not too keen on implementing the per-region version. It seems to be a bit more "stateful" than the other kinds, and seems to have more special cases. But I wouldn't be opposed if someone did a PR :-)

-- brittany-next-declaration --indent=4 is indeed nicer, and -- brittany-disable-next-declaration is nice too - I had not really considered the "disabling" part yet.

I am not too keen on implementing the per-region version. It seems to be a bit more "stateful" than the other kinds, and seems to have more special cases. But I wouldn't be opposed if someone did a PR :-) `-- brittany-next-declaration --indent=4` is indeed nicer, and `-- brittany-disable-next-declaration` is nice too - I had not really considered the "disabling" part yet.

eborden commented

2017-10-02 21:06:15 +02:00

(Migrated from github.com)

It might also be nice to specify YAML paths instead of command line args. That way configuration files and inline declarations remain consistent.

-- brittany-next-declaration conf_layout.lconfig_indentAmount: 2

It might also be nice to specify `YAML` paths instead of command line args. That way configuration files and inline declarations remain consistent. ``` -- brittany-next-declaration conf_layout.lconfig_indentAmount: 2 ```

👍 1

lspitzner commented

2017-10-02 21:18:36 +02:00

(Migrated from github.com)

make that

-- brittany-next-declaration conf_layout: {lconfig_indentAmount: 2}

and the payload is a valid YAML document, so the existing parser can be used just like I reused the commandline config parser.

make that ~~~~ -- brittany-next-declaration conf_layout: {lconfig_indentAmount: 2} ~~~~ and the payload is a valid YAML document, so the existing parser can be used just like I reused the commandline config parser.

👍 1

eborden commented

2017-10-02 21:30:01 +02:00

(Migrated from github.com)

That sounds fantastic to me!

lspitzner commented

2018-04-17 20:15:59 +02:00

(Migrated from github.com)

see #136