Make CommentGroups AST-children of Files#249
Conversation
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
Looks reasonable. Well done for figuring out all about update scripts! I have a few minor comments.
| from CommentGroup cg, File f, int idx | ||
| where | ||
| f = getLocation(cg).getFile() and | ||
| rank[idx](CommentGroup rankedcg | |
There was a problem hiding this comment.
I think this should probably be idx + 1 (unlike many other things in QL, rank is 1-based, for Reasons).
| class CommentGroup extends @comment_group, Locatable { | ||
| Comment getComment(int i) { comments(result, _, this, i, _) } | ||
|
|
||
| Comment getAComment() { result = getComment(_) } |
There was a problem hiding this comment.
This predicate isn't used anywhere, is it?
| EntityColumn(CommentGroupType, "id").Key(), | ||
| EntityColumn(FileType, "parent"), | ||
| IntColumn("idx"), | ||
| ) |
There was a problem hiding this comment.
It may be worth adding .KeySet("parent", "idx") to record the fact that there is only a single comment group at any given index in a single file. (There may be other children at the same index, but they aren't comment groups and hence not recorded in this table.)
smowton
force-pushed
the
smowton/feature/comment-group-ast-node-parents
branch
from
035f34d to
99bd4a4
Compare
|
@max-schaefer done |
|
LGTM, but you'll need to update the copy of the (new) dbscheme in the |
smowton
force-pushed
the
smowton/feature/comment-group-ast-node-parents
branch
from
99bd4a4 to
0acdcc1
Compare
|
Doh right, done |
|
Needs conflict resolution. |
Previously they were roots, with children hanging off them. Now they are children of Files, and both CommentGroups and Comments can be discovered using AstNode.getAChild. The PrintAst pass is also adapted to account for their new position.
smowton
force-pushed
the
smowton/feature/comment-group-ast-node-parents
branch
from
0acdcc1 to
6bf3802
Compare
|
Rebase'd |
| // extract a pseudo `@commentgroup` for each expr that contains their associated comments | ||
| grouplbl := tw.Labeler.LocalID(GoModExprCommentWrapper{expr}) | ||
| dbscheme.CommentGroupsTable.Emit(tw, grouplbl) | ||
| dbscheme.CommentGroupsTable.Emit(tw, grouplbl, tw.Labeler.FileLabel(), 0) |
There was a problem hiding this comment.
@smowton, looking at this code again, wouldn't this put every comment group in a go.mod file at index zero?
There was a problem hiding this comment.
Damnit, I thought this was called once per file, whereas actually it hits the toplevel comments and also comments relating to each expr.
There was a problem hiding this comment.
OK, would be good to fix that, since our dbscheme claims that the tuple (file, index) functionally determines the comment group.
There was a problem hiding this comment.
I was under the impression that
// some doc comment
require blah
Associated that comment with the require line, so it would be possible to have multiple. I hadn't actually tested this theory, though.
4 participants
Previously they were roots, with children hanging off them. Now they are children of Files, and both CommentGroups and Comments can be discovered using AstNode.getAChild.
The PrintAst pass is also adapted to account for their new position.