-
Notifications
You must be signed in to change notification settings - Fork 97
Conversation
d84a19b
to
70059b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I agree that the GetSourceFileSource
rule seems unnecessary. As for tests, maybe we could just spin up ghcide to generate the file and read it afterwards in a test and see if it contains the source?
70059b9
to
3373b25
Compare
I think there is no real way of knowing the hie file location from outside ghcide. It looks to me like we would need unit-tests that can directly access inner rules to really test it. |
src/Development/IDE/Core/Rules.hs
Outdated
case msource of | ||
Nothing -> do | ||
source <- readFile' (fromNormalizedFilePath nfp) | ||
let bsSource = T.encodeUtf8 $ T.pack source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a very roundabout way of reading a file: String -> Text -> BS.
Why not use BS.readFile
like GHC does?
https://gitlab.haskell.org/ghc/ghc/-/blob/master/compiler/GHC/Iface/Ext/Ast.hs#L284
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thank you!
Side question, is there some invariant that this filepath exists in the filesystem? Do we need more exception handling here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think existing error handling will deal with this if the file does not exist.
3373b25
to
576054c
Compare
I think a better way to achieve this would be to save the file contents along with the parsed module, so that we can be sure that it is never out of sync. This can be done along with the |
The Shake graph already guarantees this property I think |
rebase needed |
576054c
to
a1af229
Compare
a1af229
to
e2cb33d
Compare
Closes #671
Only manual testing done, yet. Not sure how this can be tested, suggestions welcome!