Instance methods don't seem to be formatted #17

New Issue

2017-03-24T20:02:51+01:00

ElvishJerricco commented

2017-03-24 20:02:51 +01:00

(Migrated from github.com)

data Test = Test

instance Show Test where
  show Test = "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test"

Doesn't seem to be reformatted at all.

``` data Test = Test instance Show Test where show Test = "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ++ "Test" ``` Doesn't seem to be reformatted at all.

👍 8

lspitzner commented

2017-03-28 19:10:15 +02:00

(Migrated from github.com)

Well, it is accurately described in the README

other module elements (data-decls, classes, instances, imports/exports etc.) are not transformed in any way; this extends to e.g. bindings inside class instance definitions

But that certainly (only) is a description of the current state of things, and implementing support for instances is on the to-do list.

Well, it is accurately described in the README > other module elements (data-decls, classes, instances, imports/exports etc.) are not transformed in any way; this extends to e.g. bindings inside class instance definitions But that certainly (only) is a description of the current state of things, and implementing support for instances is on the to-do list.

👍 2

tfausak commented

2018-05-22 14:50:32 +02:00

(Migrated from github.com)

I would love to have Brittany be able to format classes and instances! How can I help here?

lspitzner commented

2018-05-23 18:53:39 +02:00

(Migrated from github.com)

Implementation-wise, the task is not much different from supporting data-type declarations: Take some class or instance definition, consider its AST and add the corresponding case to layoutDecl (which by now lives in Language.Haskell.Brittany.Internal.Layouters.Decl) and handle the GHC-AST -> (To)BriDocconversion. See my comment of this process over at the data-type issue.

For both classes and instances we will of course re-use the existing layouters for signatures and bindings, but the headers will need new layouters. So the layouters will probably both have some form like

layoutClass (HsClass .. header .. decls ..) = docAddBaseY BrIndentRegular $ docPar
  (layoutClassHeader header)
  (docLines $ layoutDecl <$> decls)

(overly simplified)

Implementation is just one step; a rough roadmap might look like:

Take some very simple test-case and write a layouter that can handle it, confirm that it works. One class and one instance perhaps.
Brain-storm one or multiple most-complex input cases.
Consider the desired layout for these cases.
Decide what cases are to be supported. For example I have decided to not support multiple-line patterns for now. There are better ways to handle such stuff for classes/instances: We can fall back on exactprint, presuming we can distinguish these cases in the AST.
Add tests for all stuff we want to support
Write the corresponding layouting functions.
Add some more tests by inserting comments in various (even uncommon) places and see that they work properly. Potentially consider if comments should affect which layouts are used.
Check if things work with different compiler/GHC-API version.

Very simple input would e.g. be

class MyClass x where
  myMethod :: x ->  x

instance MyClass Int where
  myMethod x = x +  1

(The unnecessary whitespace will indicate if things are working.)

For something very complex, consider something like

class
  ( SuperClass1 a b c
  , SuperClass2 a b c
  , SuperClass3 a b c
  , SuperClass4 a b c
  ) => MyClass a b c
  | a -> b, a -> c
  where
    func :: a -> b -> c
    default func :: Monoid c => a -> b -> c
    func = _

(this might have a better layout, just some example to serve as input.)
(And yes, we assume -XMultiParamTypeClasses and -XFunctionalDependencies here. Feel free to not support any extensions that are hard to design/implement - better get support for 2010 Haskell than get stuck with those.)

Implementation-wise, the task is not much different from supporting data-type declarations: Take some class or instance definition, consider its AST and add the corresponding case to `layoutDecl` (which by now lives in `Language.Haskell.Brittany.Internal.Layouters.Decl`) and handle the `GHC-AST -> (To)BriDoc`conversion. See [my comment of this process over at the data-type issue](https://github.com/lspitzner/brittany/issues/47#issuecomment-322287225). For both classes and instances we will of course re-use the existing layouters for signatures and bindings, but the headers will need new layouters. So the layouters will probably both have some form like ~~~~ layoutClass (HsClass .. header .. decls ..) = docAddBaseY BrIndentRegular $ docPar (layoutClassHeader header) (docLines $ layoutDecl <$> decls) ~~~~ (overly simplified) Implementation is just one step; a rough roadmap might look like: 1. Take some very simple test-case and write a layouter that can handle it, confirm that it works. One class and one instance perhaps. 2. Brain-storm one or multiple most-complex input cases. 3. Consider the desired layout for these cases. 4. Decide what cases are to be supported. For example I have decided to not support multiple-line patterns for now. There are better ways to handle such stuff for classes/instances: We can fall back on exactprint, presuming we can distinguish these cases in the AST. 5. Add tests for all stuff we want to support 6. Write the corresponding layouting functions. 7. Add some more tests by inserting comments in various (even uncommon) places and see that they work properly. Potentially consider if comments should affect which layouts are used. 8. Check if things work with different compiler/GHC-API version. ---- Very simple input would e.g. be ~~~~ class MyClass x where myMethod :: x -> x instance MyClass Int where myMethod x = x + 1 ~~~~ (The unnecessary whitespace will indicate if things are working.) For something very complex, consider something like ~~~~ class ( SuperClass1 a b c , SuperClass2 a b c , SuperClass3 a b c , SuperClass4 a b c ) => MyClass a b c | a -> b, a -> c where func :: a -> b -> c default func :: Monoid c => a -> b -> c func = _ ~~~~ (this might have a better layout, just some example to serve as input.) (And yes, we assume `-XMultiParamTypeClasses` and `-XFunctionalDependencies` here. Feel free to not support any extensions that are hard to design/implement - better get support for 2010 Haskell than get stuck with those.)

👍 1

ElvishJerricco commented

2018-07-19 22:49:05 +02:00

(Migrated from github.com)

Any chance we could get just the instance body for now? That at least seems trivial. The harder problem of the instance head can be solved after, right?

👍 2

eborden commented

2018-07-20 01:54:09 +02:00

(Migrated from github.com)

Instance body is a great incremental step. There may be some blockers since instance syntax can get pretty flexible...

instance Foo Bar where foo = my method body

Instance body is a great incremental step. There may be some blockers since `instance` syntax can get pretty flexible... ``` instance Foo Bar where foo = my method body ```

lspitzner commented

2018-07-20 22:22:05 +02:00

(Migrated from github.com)

I'd approach this using the following technique: Take the instance ast, remove all children from it, and pass that to exactprint. With some luck this gets us just the unmodified head including any comments connected to it. Then use layoutDecl on the children we removed, and wrap the resulting list of BriDocs to add the desired indentation.

(Whole modules were done in the same manner, before we had head/exports/imports formatting.)

I'd approach this using the following technique: Take the instance ast, remove all children from it, and pass that to exactprint. With some luck this gets us just the unmodified head including any comments connected to it. Then use `layoutDecl` on the children we removed, and wrap the resulting list of BriDocs to add the desired indentation. (Whole modules were done in the same manner, before we had head/exports/imports formatting.)

ruhatch commented

2018-10-09 17:00:40 +02:00

(Migrated from github.com)

I'm working on a PR for this now - it's working with the stack.yaml, so just need to test it with other versions of GHC.

Right now it's just for instances and calls through to ExactPrint in a couple of places. I'm concerned about data definitions in instances, because the ExactPrint results contain newlines and indentation. I think I might have to implement a layouter for data definitions too, otherwise you end up doing some Text processing on the resulting BDFExternal, which is not ideal.

I can put a PR up with the Text processing stuff in place and maybe you can take a look, @lspitzner, and let me know if you can think of any other simple solutions.

I'm working on a PR for this now - it's working with the `stack.yaml`, so just need to test it with other versions of GHC. Right now it's just for instances and calls through to `ExactPrint` in a couple of places. I'm concerned about `data` definitions in instances, because the `ExactPrint` results contain newlines and indentation. I think I might have to implement a layouter for data definitions too, otherwise you end up doing some `Text` processing on the resulting `BDFExternal`, which is not ideal. I can put a PR up with the `Text` processing stuff in place and maybe you can take a look, @lspitzner, and let me know if you can think of any other simple solutions.

👍 1 ❤️ 1 🎉 1

eborden commented

2018-10-09 17:56:16 +02:00

(Migrated from github.com)

@ruhatch I'd happily test this against Freckle's codebase when you have the branch up. Thanks for the work!

ruhatch commented

2018-10-10 20:41:36 +02:00

(Migrated from github.com)

The PR is up and linked above.

@eborden Running that over Freckle's codebase and carefully checking any complex instances would be great.

@lspitzner If you could take a look at the stripWhitespace' function and let me know if you can see any better ways of doing it, that'd be awesome :)

The PR is up and linked above. @eborden Running that over Freckle's codebase and carefully checking any complex instances would be great. @lspitzner If you could take a look at the `stripWhitespace' ` function and let me know if you can see any better ways of doing it, that'd be awesome :)

👍 1

eborden commented

2018-10-10 21:08:23 +02:00

(Migrated from github.com)

@ruhatch Output looks great to me. I'm only seeing 3 additional brittany errors, but those are all coming from instance bodies that include splices, that were previously ignored. Great work!

@ruhatch Output looks great to me. I'm only seeing 3 additional `brittany` errors, but those are all coming from instance bodies that include splices, that were previously ignored. Great work!

tfausak commented

2019-06-18 15:07:42 +02:00

(Migrated from github.com)

This was fixed by #187. Thanks @ruhatch!

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#17