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

Format all files in src/ by cabal run -- src/**/*.hs #599

Closed
wants to merge 6 commits into from

Conversation

toku-sa-n
Copy link
Collaborator

For #598.

The purpose is
- To test `HIndent` with an actual project.
- To minimize commit diffs. This commit's one is huge, but then
consecutive diffs should be minimized because all sources are formatted
in the same way.
See mihaimaruseac#600. The second one
is not affected, but I have decided to remove it. Having a headline at
the middle of a list and not having at the top of it is somewhat
strange.
@toku-sa-n
Copy link
Collaborator Author

@mihaimaruseac If you have time, could you look at this PR (and #598 if ok)? This PR should reduce a few diffs in #593 because some of the diffs in the PR are due to the formatter application. Of course, I can remove these diffs from the PR, though.

Copy link
Owner

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I only had a few minor suggestions to keep the same intent behind comments.

Comment on lines +3 to +4
{-# LANGUAGE OverloadedStrings, ScopedTypeVariables, PatternGuards
#-}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's split these to one per line

Comment on lines +8 to +17
( reformat
, prettyPrint
, parseMode
, test
, testFile
, testAst
, testFileAst
, defaultExtensions
, getExtensions
) where
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the comments for separation here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the first comment because of #600 and the rest because I thought it was somewhat strange to have headlines in the middle of a list and not have one at the first line. Should we first resolve #600?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, I'd prefer to keep the documentation blocks


--------------------------------------------------------------------------------
-- Extensions stuff stolen from hlint

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this empty line. The above comment is for an entire section in the file whereas the one below is just for the function

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, HIndent doesn't preserve empty lines between comments; fixing it requires modifying the source code, and I don't think it's an easy task.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, let's open an issue for this too (if there isn't any)


--------------------------------------------------------------------------------
-- Comments

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, let's keep this empty line


--------------------------------------------------------------------------------
-- * Pretty printing class

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need this emtpy line

instance Pretty DataOrNew where
prettyInternal = pretty'

instance Pretty FunDep where
prettyInternal = pretty'

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this one, we're just adding a new instance in some conditions

#if !MIN_VERSION_haskell_src_exts(1,21,0)
instance Pretty Kind where
prettyInternal = pretty'
#endif

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep

Special _ s -> pretty s


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep

@@ -2026,10 +2135,9 @@ recUpdateExpr expWriter updates = do

--------------------------------------------------------------------------------
-- Predicates

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep

@@ -70,46 +79,37 @@ data Config = Config
, configExtensions :: [Extension]
-- ^ Extra language extensions enabled by default.
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this line

@toku-sa-n
Copy link
Collaborator Author

Thank you for the review. Unfortunately, most suggestions can't be applied due to the current HIndent implementation. Do I modify the current implementation to apply them, or close this PR and include them in #593?

@mihaimaruseac
Copy link
Owner

Let's keep it open for now and wait on fixing the blocking issues.

For #593, let's go ahead with it without focusing on auto-formatting for now, unless required by testing.

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.

2 participants