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

Amend fix for correct placement of file header pragmas (#1958) #2078

Merged
merged 2 commits into from
Aug 8, 2021

Conversation

nini-faroux
Copy link
Contributor

hi again, I noticed two issues with the previous fix.

The first was the case where the pragma types were interchanging, like: 
{-# OPTIONS_GHC ... #-}
{-# LANGUAGE ... #-}
{-# LANGUAGE ... #-} -- newly added, should be last
{-# OPTIONS_GHC ... #-}

Here the line number for the last LANGUAGE pragma was always calculated last and used even if it wasn't the largest of the three. This is fixed now, by taking the max.

The other issue was when cleaning things up last time I changed the 'afterShebang' function to avoid some duplication, the tests didn't have a case to check for this but it worked out that when there is no Language pragma at the top of the file this would basically match any type of pragma later in the file instead of none. This is fixed now just by calculating the shebang separately to LANGUAGE and OPTIONS_GHC.

I added a bunch of new tests too just for different types of interchanging and ignoring later pragmas like the BangPatterns example in the original issue.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Thank you so much for the fixes and the amazing test suite.

GHC has a parser for module headers (https://hackage.haskell.org/package/ghc-8.10.2/docs/Parser.html#v:parseHeader) that might be worth considering if/when our custom logic grows out of control. But we're not there yet, and I'm not sure the GHC parser would cover all the cases.

@nini-faroux
Copy link
Contributor Author

Oh cool, I'll have a look at the parser. I think this covers a number of corner cases hopefully but I'm sure there's others that will come up too. If I think of any more I'll add them.

Thanks again!

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Aug 8, 2021
@mergify mergify bot merged commit b8b9620 into haskell:master Aug 8, 2021
@nini-faroux nini-faroux deleted the pragmafix branch August 10, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants