Use auto-formatting (hindent?)

bergmark commented 7 years ago

I think auto-formatting is a big win. hindent also formats very close to what I do personally anyway so I'm a big fan, but there are a lot of differences from the aeson style.

What do others think?

phadej commented 7 years ago

If we can autocheck hindent in Travis, than why not. There was something I didn't liked in hindent style, but OTOH the style debate won't never converge to consensus anyway. IIRC hindent formatting is diff friendly, which is most important for me anyway.

alexeyzab commented 7 years ago

Hi there! Is this issue still relevant? If so, would any of the maintainers be interested in providing some mentorship/general help on resolving this so that the newcomers and first-time contributors would have an easier time?

I would like to add this as one of the issues for the upcoming Haskell Weekly's Call for Participation section. See discussion in #75.

These are the guidelines we'd like to stick to in the future:

Ensure that your project has at least one open-source licence. (we've decided to not define the term "open-source" and it is left to your own interpretation).
Ensure that the issue tracker for your project is publicly accessible.
Create a new issue in your issue tracker and clearly describe the task. Also mention the difficulty level (easy/medium/hard/tedious), either as a tag/label or somewhere in the title/description.
If you have specific requirements for contributors (e.g., copyright waiver), it must be mentioned in the description of the task (preferably with a link to CONTRIBUTING.md).

Thank you!

bergmark commented 7 years ago

Hi @alexeyzab,

We're always happy to provide help with contributions.

I think this would be a good beginner-level task as it shouldn't modify behavior of the library. I think we satisfy all of your guidelines. Let me know if I'm missing something.

TODO list:

Run hindent on the entire code base
Add a formatting step to the Makefile
Add a Travis CI step to .travis.yml and travis/script.sh that verifies formatting. See if there's a binary available for download, otherwise install with stack install hindent --resolver nightly
Replace style guide in CONTRIBUTING.md with a reference to hindent

Additional/optional tasks:

Are there any configuration options for hindent we should decide on?
There are alternative formatters like hfmt and brittany, are any of them worth considering?

lorenzo commented 7 years ago

@bergmark Currently it is not possible to use hindent with certain files using the CPP language extension. Would it be OK to skip those files in the formatting while this known bug is fixed?

If it is acceptable I can take on this task

phadej commented 7 years ago

@lorenzo related: https://github.com/bos/aeson/issues/529

bergmark commented 7 years ago

Re. the travis step, is it possible to have it not break the build? I'm not sure if we want to force contributors to install hindent. I could do the formatting before making a release instead.

lorenzo commented 7 years ago

Yes, it is possible

lorenzo commented 7 years ago

There are important bugs that need to be fixed in hindent before this is done. This one is very serious https://github.com/commercialhaskell/hindent/issues/433

bergmark commented 7 years ago

Alright, thanks for looking into this @lorenzo!

pbrinkmeier commented 7 months ago

Hi, I'd like to try and push this forward as first issue. I tried three formatters (hindent, fourmulo and stylish-haskell) and all of them have problems with syntax forms that are interrupted by CPP directives, e.g. the module header in src/Data/Aeson/Internal/ByteString.hs:

module Data.Aeson.Internal.ByteString (
    mkBS, 
    withBS,
#ifdef MIN_VERSION_template_haskell
    liftSBS,
#endif
) where

or tests in tests/PropertyGeneric.hs:

genericTests :: TestTree                                                                                                                                                                                    
genericTests =                                                                                                                                                                                              
  testGroup "generic" [                                                                                                                                                                                     
      testGroup "toJSON" [                                                                                                                                                                                  
        testGroup "Nullary" [ ... ]
        ...                                                                                                                                                                                                       
#if !MIN_VERSION_base(4,16,0)                                                                                                                                                                               
      , testGroup "OptionField" [                                                                                                                                                                           
          testProperty "like Maybe" $                                                                                                                                                                       
          \x -> gOptionFieldToJSON (OptionField (Option x)) === thMaybeFieldToJSON (MaybeField x)                                                                                                           
        , testProperty "roundTrip" (toParseJSON gOptionFieldParseJSON gOptionFieldToJSON)                                                                                                                   
        ]                                                                                                                                                                                                   
#endif                                                                                                                                                                                                      
      ]
    ]

So in order to use any of these formatters these cases would probably have to be eliminated. I also encountered problems with hindent and Template Haskell, but haven't tested if they are present for the other formatters. If you think it would be cool to have automatic formatting let me know and I can investigate some more and create a PR that restructures the code where necessary and applies the formatting :)

phadej commented 6 months ago

have to be eliminated.

The compatibility CPP is unfortunately a MUST for a library like aeson. I'm not aware you can cheaply eliminate it without sacrificing something (like support window width, which is not an option).

Lysxia commented 6 months ago

I think the idea is not to get rid of CPP altogether, but to rewrite the CPP so that the chosen formatter can handle it, or make some feature requests to the formatter.

phadej commented 6 months ago

The

genericTests :: TestTree                                                                                                                                                                                    
genericTests =                                                                                                                                                                                              
  testGroup "generic" [                                                                                                                                                                                     
      testGroup "toJSON" [                                                                                                                                                                                  
        testGroup "Nullary" [ ... ]
        ...                                                                                                                                                                                                       
#if !MIN_VERSION_base(4,16,0)                                                                                                                                                                               
      , testGroup "OptionField" [                                                                                                                                                                           
          testProperty "like Maybe" $                                                                                                                                                                       
          \x -> gOptionFieldToJSON (OptionField (Option x)) === thMaybeFieldToJSON (MaybeField x)                                                                                                           
        , testProperty "roundTrip" (toParseJSON gOptionFieldParseJSON gOptionFieldToJSON)                                                                                                                   
        ]                                                                                                                                                                                                   
#endif                                                                                                                                                                                                      
      ]
    ]

is the code which should work. I don't want to compromise on a way the code is structured now.

I'm not unhappy because of current (inconsistent) formatting, and I'm not so happy with outputs of any formatter that I'd like to sacrifice more than just looking at their ugly outputs.

pbrinkmeier commented 6 months ago

Point taken.

I think the idea is not to get rid of CPP altogether, but to rewrite the CPP so that the chosen formatter can handle it, or make some feature requests to the formatter.

This is exactly what I meant but I see that you have a coding style you would like to stick to. I will check for another issue to tackle (open for suggestions).

haskell / aeson

Use auto-formatting (hindent?) #527