Alternative rules for signatures #94

New Issue

2017-12-28T13:21:05+01:00

lierdakil commented

2017-12-28 13:21:05 +01:00

(Migrated from github.com)

This is probably a stupid question, but nonetheless. Some editors (mostly relying on TextMate grammar model) have trouble with highlighting signatures correctly if :: operator appears on a different line than identifier itself. FWIW, GitHub also has this problem.

Compare:

a.

replaceAttr
  :: Options
  -> Either String String
  -> Maybe String
  -> Inlines
  -> String
  -> WS Inlines

b.

replaceAttr :: Options
            -> Either String String
            -> Maybe String
            -> Inlines
            -> String
            -> WS Inlines

The question is, can I somehow convince brittany to format this as (b) instead of (a)?

This is probably a stupid question, but nonetheless. Some editors (mostly relying on TextMate grammar model) have trouble with highlighting signatures correctly if `::` operator appears on a different line than identifier itself. FWIW, GitHub also has this problem. Compare: a. ```hs replaceAttr :: Options -> Either String String -> Maybe String -> Inlines -> String -> WS Inlines ``` b. ```hs replaceAttr :: Options -> Either String String -> Maybe String -> Inlines -> String -> WS Inlines ``` The question is, can I somehow convince brittany to format this as (b) instead of (a)?

lspitzner commented

2017-12-28 21:56:16 +01:00

(Migrated from github.com)

@lierdakil There is no config option for such behaviour. But it seemed trivial to implement, so I quickly added 37e355fea5. Could you test the hangingtypesig branch? you'll have to add the new config flag to your user/project brittany config file (lconfig_hangingTypeSignature: true).

Disclaimer: I haven't written proper tests, and won't promise that this works in general or that this will be maintained in the future, because maintaining dozens of layouting special case config options such as this one will become rather time consuming.

@lierdakil There is no config option for such behaviour. But it seemed trivial to implement, so I quickly added 37e355fea57133302f0fc4b27cc358b5b3745466. Could you test the [`hangingtypesig`](https://github.com/lspitzner/brittany/tree/hangingtypesig) branch? you'll have to add the new config flag to your user/project brittany config file (`lconfig_hangingTypeSignature: true`). Disclaimer: I haven't written proper tests, and won't promise that this works in general or that this will be maintained in the future, because maintaining dozens of layouting special case config options such as this one will become rather time consuming.

🎉 2

lierdakil commented

2017-12-29 00:33:16 +01:00

(Migrated from github.com)

But it seemed trivial to implement, so I quickly added 37e355f.

Thanks!

Could you test the hangingtypesig branch?

Didn't break on my sources, so seems to work

I haven't written proper tests

Let me know if you want me to make a PR for that

won't promise that this works in general or that this will be maintained in the future

I would hope that I made a reasonable enough case for why would one want this particular special case. Pretty sure Atom, TextMate and VSCode all have this limitation, and as you can plainly see, so does GitHub itself (that's because it uses Atom's grammar actually, but truth is there's no good alternative to that). Hopefully the deficiency of the grammar model that leads to this problem will be fixed sometime in the future, but in the meantime having :: put on the second line by prettifiers is rather annoying.

Full disclosure, I'm maintaining Haskell grammar for Atom, so I'm obviously biased -- I want brittany (and prettifiers in general) to play nice with that.

> But it seemed trivial to implement, so I quickly added 37e355f. Thanks! > Could you test the hangingtypesig branch? Didn't break on my sources, so seems to work > I haven't written proper tests Let me know if you want me to make a PR for that > won't promise that this works in general or that this will be maintained in the future I would hope that I made a reasonable enough case for *why* would one want this particular special case. Pretty sure Atom, TextMate and VSCode all have this limitation, and as you can plainly see, so does GitHub itself (that's because it uses Atom's grammar actually, but truth is there's no good alternative to that). Hopefully the deficiency of the grammar model that leads to this problem will be fixed sometime in the future, but in the meantime having `::` put on the second line by prettifiers is rather annoying. Full disclosure, I'm maintaining [Haskell grammar for Atom](https://github.com/atom-haskell/language-haskell), so I'm obviously biased -- I want brittany (and prettifiers in general) to play nice with that.

eborden commented

2017-12-29 07:00:14 +01:00

(Migrated from github.com)

@lierdakil Is there anything blocking a fix in Atom's grammar? I also share @lspitzner's fear of ever expanding layout options. This is likely one that people want, but I've seen the \n :: style pretty pervasively in the Haskell ecosystem and this should likely be fixed in the upstream tool as well.

@lierdakil Is there anything blocking a fix in Atom's grammar? I also share @lspitzner's fear of ever expanding layout options. This is likely one that people want, but I've seen the `\n ::` style pretty pervasively in the Haskell ecosystem and this should likely be fixed in the upstream tool as well.

lierdakil commented

2017-12-29 07:19:54 +01:00

(Migrated from github.com)

@eborden yes. It's not a problem with grammar itself, it's a limitation of underlying grammar engine. Here's a TL;DR version: TextMate's grammar model (used also by Atom and VSCode) tokenizes line-by-line and forbids backtracking. Latter is for performance reasons. Hence, no lookaheads beyond current line. Also no arbitrary "state". See referencing issues for more background.

I have some hopes for tree-sitter, which is a proper parsing grammar engine for Atom and is apparently going to be available soon™ (although I believe nobody knows when exactly), but that won't help with other editors.

@eborden yes. It's not a problem with grammar itself, it's a limitation of underlying grammar engine. Here's a TL;DR version: TextMate's grammar model (used also by Atom and VSCode) tokenizes line-by-line and forbids backtracking. Latter is for performance reasons. Hence, no lookaheads beyond current line. Also no arbitrary "state". See referencing issues for more background. I have some hopes for [tree-sitter](https://github.com/atom/tree-sitter-syntax), which is a proper parsing grammar engine for Atom and is apparently going to be available soon™ (although I believe nobody knows when exactly), but that won't help with other editors.

lspitzner commented

2017-12-29 16:56:23 +01:00

(Migrated from github.com)

@lierdakil My editor (sublime-text) breaks too, but I value (visual) consistency (in indentation) and not wasting space for frequently large type signatures over some text colouring.

I would hope that I made a reasonable enough case for why would one want this particular special case.

Well, and I created the commit, did I not? You seem to be missing my point. If 10 users come along with well-reasoned requests for special cases, it still might be too much work for me to maintain the corresponding code, and I wish to clearly communicate this. Repeating the reasoning for your case does not help with that.

Thanks for offering to contribute some tests, but this is blocked until we finish/merge the inlineconfig branch. Without that, testing custom configs is rather cumbersome.

The details of why these highlighting engines break is interesting, and it is good to hear of better tooling on the horizon.

I'll merge hangingtypesig into dev soon.

@lierdakil My editor (sublime-text) breaks too, but I value (visual) consistency (in indentation) and not wasting space for frequently large type signatures over some text colouring. > I would hope that I made a reasonable enough case for why would one want this particular special case. Well, and I created the commit, did I not? You seem to be missing my point. If 10 users come along with well-reasoned requests for special cases, it still might be too much work for me to maintain the corresponding code, and I wish to clearly communicate this. Repeating the reasoning for your case does not help with that. Thanks for offering to contribute some tests, but this is blocked until we finish/merge the `inlineconfig` branch. Without that, testing custom configs is rather cumbersome. The details of why these highlighting engines break is interesting, and it is good to hear of better tooling on the horizon. I'll merge `hangingtypesig` into `dev` soon.

lierdakil commented

2017-12-29 18:25:05 +01:00

(Migrated from github.com)

Preface: it's your project, so your word is final, obviously. I can only give suggestions and try to explain reasoning behind those. Which is exactly what I did, or at least I hope so.

wasting space

I would argue that (b) in OP "wastes" less space than (a), if we consider vertical space as well, but that's a matter of taste more than anything else.

I created the commit, did I not?

And I'm very grateful, even if I might be bad at communicating that.

If 10 users come along with well-reasoned requests for special cases

I personally find that "because tooling" is a stronger reason than "because style guide", but you're free to disagree of course. I can't think of a lot of "because tooling" reasons from the top of my head, so I would hope there won't be a lot of special cases like this. I guess it boils down to this request: if you find you have to remove support for some special cases to keep brittany maintainable, bear in mind that without this particular one, stuff breaks.

I'll merge hangingtypesig into dev soon.

Thanks again!

Preface: it's your project, so your word is final, obviously. I can only give suggestions and try to explain reasoning behind those. Which is exactly what I did, or at least I hope so. > wasting space I would argue that (b) in OP "wastes" *less* space than (a), if we consider vertical space as well, but that's a matter of taste more than anything else. > I created the commit, did I not? And I'm very grateful, even if I might be bad at communicating that. > If 10 users come along with well-reasoned requests for special cases I personally find that "because tooling" is a stronger reason than "because style guide", but you're free to disagree of course. I can't think of a lot of "because tooling" reasons from the top of my head, so I would hope there won't be a lot of special cases like this. I guess it boils down to this request: if you find you have to remove support for some special cases to keep `brittany` maintainable, bear in mind that without this particular one, stuff breaks. > I'll merge hangingtypesig into dev soon. Thanks again!

lspitzner commented

2018-02-14 18:42:17 +01:00

(Migrated from github.com)

@lierdakil I still wanted to say thanks for reporting this issue and for explaining the reasoning for wanting this feature. It is vital that users give input so that maintainers can make sensible decisions, and I don't want to take away from this. So please don't let my pushing back above discourage you from reporting things in the future.

I finally did a release 0.9.0.1 that contains the additional config flag.

@lierdakil I still wanted to say thanks for reporting this issue and for explaining the reasoning for wanting this feature. It is vital that users give input so that maintainers can make sensible decisions, and I don't want to take away from this. So please don't let my pushing back above discourage you from reporting things in the future. I finally did a release `0.9.0.1` that contains the additional config flag.

Sign in to join this conversation.

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#94