Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix J.Tibell style for record syntax: add line before '}' #4

Merged
merged 1 commit into from
Oct 3, 2014

Conversation

PierreR
Copy link
Contributor

@PierreR PierreR commented Oct 3, 2014

https://github.com/tibbe/haskell-style-guide/blob/master/haskell-style.md

data Person = Person
    { firstName :: !String  -- ^ First name
    , lastName  :: !String  -- ^ Last name
    , age       :: !Int     -- ^ Age
    } deriving (Eq, Show)

@chrisdone
Copy link
Collaborator

Good fix! I'd only tested it with comments which auto-inserts the newline.

@chrisdone chrisdone merged commit 795b1c2 into mihaimaruseac:master Oct 3, 2014
@PierreR
Copy link
Contributor Author

PierreR commented Oct 14, 2014

Hi, I would like to open an issue about the lack of composability of the current "API" but issues are closed :-(

For instance what is the best way to reuse your formatting for function signature in another style (such a Tibell that doesn't currently case on function signature).

Looking at how @ocharles is achieving this same goal, you can see that it is not optimal. On the other, copy-paste is not great either.

@chrisdone
Copy link
Collaborator

@PierreR There are two options:

  • Make my ChrisDone module export a prettyTypeSig function.
  • Just copy/paste the code.

In the first choice, you can still customize the behaviour by extending sub-node-types.

I may prefer the latter choice because I do not want people relying on behaviour in my ChrisDone module if I decide I want to change how type-sigs are formatted, unless you are ok with it changing at any time. Writing write some tests in the test suite for your style will mitigate this.

@chrisdone
Copy link
Collaborator

Looking at how @ocharles is achieving this same goal, you can see that it is not optimal. On the other, copy-paste is not great either.

What is @ocharles doing?

@chrisdone
Copy link
Collaborator

(You're free to make a PR that exports any function from my ChrisDone module or copy/paste, as you prefer. You should consider the .Style modules like people's XMonad configurations.)

@PierreR
Copy link
Contributor Author

PierreR commented Oct 15, 2014

So this gives a nice compare view that makes clear what @ocharles is doing.

ocharles/hindent@chrisdone:master...master

To re-use your definition of TypeSig, I can copy-paste it or doing something analog to what @ocharles is doing

The first case is easy. I can do a PR immediately but the copy/paste approach will quickly go out of hand.
In the latter case I cannot submit a PR because I will have to modify both HIndent.hs and ChrisDone.hs in a way that is not really compatible with the current spirit of the API (so I believe).

In a way I am wondering how @ocharles would submit a PR for his style.

I would be happy to submit a PR but I feel there is a need to give some thought to the matter.

Cheers

@ocharles
Copy link
Contributor

@ocharles has his own fork that imports the whole ChrisDone module: https://github.com/ocharles/hindent/blob/master/src/HIndent/Styles/OCharles.hs#L19

I don't see any need for module encapsulation in Style files, so I just removed the export list entirely.

@PierreR
Copy link
Contributor Author

PierreR commented Oct 15, 2014

Yes, thanks for your feedback.

That was my first try. What worries me is the fact that HIndent.hs import all the Style and then ghc is confused about which State to use (or something like that) in that file. At that point, I was not so sure it was safe to just remove encapsulation in Style (and I don't really want to create a fork just yet).

@ocharles
Copy link
Contributor

I think things should have explicit import lists, but explicit export lists is just annoying (in this scenario).

@chrisdone
Copy link
Collaborator

I'm fine with this approach.

  • Printers that don't care about the state will be type-safe to use across printers.
  • An open export list is okay as long as HIndent explicitly imports just the style to avoid name clashes.

We need a process, though. Consider the following scenario:

  • I decide that I want to tweak how dependOrNewline works or how infix expressions are rendered.
  • Now everyone depending on my style has a different style too.

Note: This is unlike the fundamental style defined in HIndent.Pretty, which should never change.

What do I do at this point? Thanks to @gibiansky, we now have a test suite. So I can theoretically know when I've broken someone else's style. I think it can be like this:

  • By using someone's internal functions, you're agreeing that it can change later.
  • That person can just break the tests and open an issue pinging you.
  • You update your style or tests as necessary as PR, we close and hackage.

Sound good? I don't think it should happen that often and it's not the end of the world, but we're busy people so it's good to know a clear way forward in that situation.

Also if some combinators seem to have nothing to do with the style in question it would ideally be moved into HIndent.Pretty which already has a bunch of handy combinators.

@chrisdone
Copy link
Collaborator

Of course, all that is assuming you guys want to share your styles, it's also possible to just have a local XMonad-like project that imports HIndent and your style. That's less maintenance burden for yourself. I think it's cool to have everyone in one place a la XMonad Contrib that users can choose simply, but just mentioning that's also an option.

@ocharles
Copy link
Contributor

I think that three step process is perfect. I'm trading a little time upfront now for potentially spending time in the future that you change how you want to do things. There's a gamble, and I should be the one paying that price if the odds stop falling in my favour.

I'm all for having a repository of common themes, ultimately I'd like to be able to tell contributors to my projects to use my style - that's much easier if they just need to cabal install hindent and use buffer local variables.

@chrisdone
Copy link
Collaborator

I think that three step process is perfect. I'm trading a little time upfront now for potentially spending time in the future that you change how you want to do things. There's a gamble, and I should be the one paying that price if the odds stop falling in my favour.

Good stuff. @PierreR also happy?

I'm all for having a repository of common themes, ultimately I'd like to be able to tell contributors to my projects to use my style - that's much easier if they just need to cabal install hindent and use buffer local variables.

Indeed, agreed. That's definitely the ideal use-case.

@PierreR
Copy link
Contributor Author

PierreR commented Oct 15, 2014

Sound great to me. Do you want me to submit a PR that removes module encapsulation at least for your Style or shall I let you do that ?

As a note, ideally I wish I would have a kind of picture to quickly see how one style fits my taste better. Personally I have no interest in developing my own style and ultimately want to pick the one that most people would use.

@chrisdone
Copy link
Collaborator

@PierreR I've explicitly imported style names in HIndent here: 6a6f366 I'll wait for @ocharles's commit regarding the module encapsulation as otherwise I'll create a merge conflict for him.

As a note, ideally I wish I would have a kind of picture to quickly see how one style fits my taste better.

To see which one fits your taste better you can just pass --style <the-style> and run it on some code. :-)

Personally I have no interest in developing my own style and ultimately want to pick the one that most people would use.

So do you want a style that fits your tastes or one that's popular? Make up your mind! ;-)

@PierreR
Copy link
Contributor Author

PierreR commented Oct 15, 2014

So do you want a style that fits your tastes or one that's popular? Make up your mind! ;-)

I would be happy with a kind of gofmt command for Haskell. In the end, I only care about readability not taste.

I am pretty sure there will be different styles for Haskell (but hopefully not too many). So now I need to pick one (hence the taste bit ;-)

To see which one fits your taste better you can just pass --style and run it on some code

Passing --style to test the various bit for each style would be quite tedious (unless I am missing something here). I believe users deserve something a bit more straightforward (like a picture) or at least a maximum of comment in code (or nice test cases)

Thanks for your input.

@gibiansky
Copy link
Collaborator

@PierreR

There are a set of test cases for the gibiansky style, which I hope will someday be extended for other styles as well. Perhaps you could look at these to see what a style looks like?

As an aside, I agree that having a reasonable "default" style is preferable, which would make hindent sort of an equivalent of gofmt. However, I don't think that any of the styles currently available are really tested well enough to be used for this. Part of the reason I wrote my test cases was to force myself to handle weird edge cases; I hope to eventually get to the point where the tests are thorough enough that they indicate a gofmt-like readiness. Sadly formatting Haskell is much harder than Go...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants