[#1958] Fix placement of language pragmas #2043
Conversation
| -- Useful for inserting pragmas. | ||
| endOfModuleHeader :: T.Text -> Range | ||
| endOfModuleHeader contents = Range loc loc | ||
| where | ||
| loc = Position line 0 | ||
| line = maybe 0 succ (lastLineWithPrefix "{-#" <|> lastLineWithPrefix "#!") | ||
| lastLineWithPrefix pre = listToMaybe $ reverse $ findIndices (T.isPrefixOf pre) $ T.lines contents | ||
| where | ||
| loc = Position line 0 | ||
| line = maybe 0 succ (lastShebang contents <|> lastOptsGhc contents <|> lastLangPrag contents) | ||
| lastShebang = lastLineWithPrefix $ T.isPrefixOf "#!" |
There was a problem hiding this comment.
Could you expand the doc comment (and fix the markup) to explain what the new behaviour is? It will make it easier to review and to understand in the future.
| checkPragma :: T.Text -> Int -> T.Text -> Bool | ||
| checkPragma name n = check | ||
| where | ||
| check l = isPragma l && getName l == name | ||
| getName l = T.take n $ T.dropWhile isSpace $ T.drop 3 l |
There was a problem hiding this comment.
It looks like the second argument is always the length of the first. Why not derive it internally if that's the case?
There was a problem hiding this comment.
hi, thanks for the quick feedback!
Ah yes good point, I'll change that function now. I also realised after that there are a few cases where this won't work as expected - for example, it will add a new Language pragma before the other language pragmas if the there is a shebang at the start of the file - like this:
#! /usr/bin/env nix-shell
{-# LANGUAGE OverloadedStrings #-} -- newly added, should be last
{-# LANGUAGE NamedFieldPuns #-}
so I was thinking it would be better to calculate the final line number as a sum the shebang, options_ghc and language pragmas. I did this locally and he works for all the older cases and new ones I could think of, like the one above. I can update my pr with this change this evening and also with the expanded comment?
There was a problem hiding this comment.
I was wondering this too - which is why I asked for more comments. Maybe it would be a good idea to add a few more tests to exercise a few more scenarios ?
There was a problem hiding this comment.
Sure no problem, I can include a few more with the update.
There was a problem hiding this comment.
So I added a number of new tests, different combinations of shebangs, ghc_options, language pragmas and later inline pragmas. I can't really think of any other combinations right now, but do let me know if I missed something or if I could improve the solution. Thanks!
There was a problem hiding this comment.
hi again, I just cleaned up the code a bit, added another test for when 'module ...' is on the first line, and also squashed it down to one commit.
|
|
||
| {-# INLINE addOne #-} | ||
| addOne :: Int -> Int | ||
| addOne x = x + 1 |
There was a problem hiding this comment.
Is this file generated? Why doesn't this name contain ".hs" extension?
There was a problem hiding this comment.
hi, no I tried the test manually first and then added it to the test suite but I must have left off the .hs, thanks for pointing it out.
|
Something I encountered today. When the module Foo where=> module Foo where
{-# LANGUAGE Blah #-}I had a quick look through the tests, but didn't see anything that tackled this case. Does it work under this PR? |
|
hi @isovector, yes it will add it above module on the first line now like - {-# LANGUAGE TupleSections #-] |
HI everyone,
I added a small fix for issue #1958, where the language pragma was being added in the wrong place. I tested it against the examples in the two issues, and checked it still works with the older tests too. Hope it's ok!