lspitzner / brittany

haskell source code formatter
GNU Affero General Public License v3.0
690 stars 72 forks source link

Whitespace added between comment and multi-line where clause #263

Closed stackptr closed 3 years ago

stackptr commented 4 years ago

It appears when formatting a function with a long comment about a multi-line where clause, brittany will add a bunch of whitespace that seems somehow related to the length of the comment.

Input this into the online formatter (some function body has been omitted for brevity):

validateBasic
  :: forall  m . (MonadThrow m, HasCallStack) => BasicQuestion -> m ()
validateBasic BasicQuestion {..} = do
  when (basicQuestionAnswerType == Just SingularOr) $ do
    let invalidChars = findAll "!@#$%^&()" basicQuestionCorrectAnswers

  -- There are a ton of other validations that would be nice to have, like:
  -- - Are the correct answers formatted properly? (Should be answers separated
  --   by space-pipe-space, like `x | y | z`.)
  -- - Do any params not get substituted? In other words, are there any non-
  --   `Nothing` params that don't have a corresponding Mustache/ERB
  --   substitution?
  -- - Do any fields other than the question text "look like" question text?
  --   The easiest way to tell would be to look for Mustache/ERB tags.
 where
  isMultiple =
    basicQuestionAnswerType `elem` [Just MultipleAnswers, Just MultipleChoice]

The output will show whitespace between the last line of the comment and the where:

validateBasic
  :: forall  m . (MonadThrow m, HasCallStack) => BasicQuestion -> m ()
validateBasic BasicQuestion {..} = do
  when (basicQuestionAnswerType == Just SingularOr) $ do
    let invalidChars = findAll "!@#$%^&()" basicQuestionCorrectAnswers

  -- There are a ton of other validations that would be nice to have, like:
  -- - Are the correct answers formatted properly? (Should be answers separated
  --   by space-pipe-space, like `x | y | z`.)
  -- - Do any params not get substituted? In other words, are there any non-
  --   `Nothing` params that don't have a corresponding Mustache/ERB
  --   substitution?
  -- - Do any fields other than the question text "look like" question text?
  --   The easiest way to tell would be to look for Mustache/ERB tags.

 where
  isMultiple =
    basicQuestionAnswerType `elem` [Just MultipleAnswers, Just MultipleChoice]

Further formatting will result in more whitespace.

The comment above is 8 lines and results in 9 lines of whitespace. Trimming the comment to 4 lines results in 5 lines of whitespace.

Shortening the definition of isMultiple (for instance, removing Just MultipleChoice from the list) will result in it formatting to a single line with no whitespace.

eborden commented 4 years ago

There also seems to be an issue here of indentation level. The code above is using special where indentation (1 space), but the comment is being indented with the do block (2 space). @stackptr what would you expect?

this

validateBasic
  :: forall  m . (MonadThrow m, HasCallStack) => BasicQuestion -> m ()
validateBasic BasicQuestion {..} = do
  when (basicQuestionAnswerType == Just SingularOr) $ do
    let invalidChars = findAll "!@#$%^&()" basicQuestionCorrectAnswers

  -- There are a ton of other validations that would be nice to have, like:
  -- - Are the correct answers formatted properly? (Should be answers separated
  --   by space-pipe-space, like `x | y | z`.)
  -- - Do any params not get substituted? In other words, are there any non-
  --   `Nothing` params that don't have a corresponding Mustache/ERB
  --   substitution?
  -- - Do any fields other than the question text "look like" question text?
  --   The easiest way to tell would be to look for Mustache/ERB tags.
 where
  isMultiple =
    basicQuestionAnswerType `elem` [Just MultipleAnswers, Just MultipleChoice]

or

validateBasic
  :: forall  m . (MonadThrow m, HasCallStack) => BasicQuestion -> m ()
validateBasic BasicQuestion {..} = do
  when (basicQuestionAnswerType == Just SingularOr) $ do
    let invalidChars = findAll "!@#$%^&()" basicQuestionCorrectAnswers

 -- There are a ton of other validations that would be nice to have, like:
 -- - Are the correct answers formatted properly? (Should be answers separated
 --   by space-pipe-space, like `x | y | z`.)
 -- - Do any params not get substituted? In other words, are there any non-
 --   `Nothing` params that don't have a corresponding Mustache/ERB
 --   substitution?
 -- - Do any fields other than the question text "look like" question text?
 --   The easiest way to tell would be to look for Mustache/ERB tags.
 where
  isMultiple =
    basicQuestionAnswerType `elem` [Just MultipleAnswers, Just MultipleChoice]
lspitzner commented 4 years ago

This is fixed on master. master on the online formatter was updated, so it works there already.

I think for the comment position nothing needs to be changed. The input is essentially already formatted, so brittany should not change anything about comment placement. Sometimes brittany changes comment indentation (e.g. if you have a comment in the middle of a do block and you switch from --indent 2 to --indent 4) but generally retaining comment position including indentation exactly is a feature.

Munksgaard commented 4 years ago

This is fixed on master. master on the online formatter was updated, so it works there already.

Would it be possible to release a new version with the fix in it?