Skip to content

[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

Closed
wants to merge 21 commits into from

Conversation

zhujinxuan
Copy link

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 be schema :: a -> (Int, Int) -> AST

@zhujinxuan
Copy link
Author

zhujinxuan commented Nov 2, 2018

screenshot 2018-11-02 01 21 15

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.

, position :: PositionInfo
} deriving (Eq,Show)

type PositionInfo = Maybe (Int, Int)
Copy link
Collaborator

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?

Copy link
Author

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" {
Copy link
Collaborator

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?

Copy link
Author

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}torecords _ 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.

@zhujinxuan
Copy link
Author

zhujinxuan commented Jan 3, 2019

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/)

@zhujinxuan zhujinxuan closed this Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants