-
Notifications
You must be signed in to change notification settings - Fork 32
[WIP] Adding Position Annotation #205
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
Conversation
Sees that the coverage test fails because we do not test `position` in record explicitly. I am not sure whether we need to test this?
Update: Commit from d004986 the coverage test with ticks overlay. |
05a6c35
to
d004986
Compare
, position :: PositionInfo | ||
} deriving (Eq,Show) | ||
|
||
type PositionInfo = Maybe (Int, Int) |
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.
Quick question: Under what circumstances would position be Nothing
? Would it make sense to add a brief comment about that?
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.
Annotation added in 0023709. Nothing
will be used for manually constructed ASTs. For example, when a client manually construct AST and encode the AST to query (for sending to server). When constructing the AST, the position information is missing.
@@ -0,0 +1,4 @@ | |||
module "GRAPHQL_API_MIX_PATH/GraphQL.Internal.Syntax.AST" { |
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.
What does this do? Is it this https://wiki.haskell.org/Haskell_program_coverage?
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.
Yes, it is an overlay of hpc. I did it because position
is not intended to be tested as function. (Haskell will translate records@{position = x}to
records _ x`, and therefore is not considered tested in HPC)
One method is to add one in hpc python script, but I think it makes more sense to use a ticks file to explicitly tell those functions we do not need to test.
6918183
to
a6dac95
Compare
Close this PR because adding position directly to AST is not a good idea. I am checking about Free or CoFree to describe the position-annotated parser. (https://www.reddit.com/r/haskell/comments/4x22f9/labelling_ast_nodes_with_locations/) |
There will be a big change if we want to add position annotation as discussed in #75
We used to have
schema :: a -> AST
, then it will beschema :: a -> (Int, Int) -> AST