X Tutup
The Wayback Machine - https://web.archive.org/web/20260215152308/https://github.com/github/codeql/pull/9744
Skip to content

Move contextual queries from src to lib#9744

Merged
aeisenberg merged 3 commits intomainfrom
aeisenberg/move-contextual-queries
Jun 29, 2022
Merged

Move contextual queries from src to lib#9744
aeisenberg merged 3 commits intomainfrom
aeisenberg/move-contextual-queries

Conversation

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Jun 28, 2022 •
edited
Loading

With this change, users are now able to run View AST command in vscode within vscode workspaces that do not include the core libraries. The relevant core library only needs to be installed in the package cache.

Question for reviewers:

  1. Does this require a change note?
  2. I am getting a failing documentation check. None of these predicates were documented before. Is there some way to skip this check for the failing predicates?

@aeisenberg aeisenberg requested review from a team as code owners June 28, 2022 21:37
@aeisenberg aeisenberg force-pushed the aeisenberg/move-contextual-queries branch from 648cbca to e05d6aa Compare June 28, 2022 22:35
@aeisenberg aeisenberg requested a review from a team as a code owner June 28, 2022 22:35
@github-actions github-actions bot added the JS label Jun 28, 2022
@jketema
Copy link
Contributor

jketema commented Jun 29, 2022

After the force push the documentation check now seems to be failing in a more fundamental way that does need to be looked at:

A fatal error occurred: Errors during binding:
ERROR: Could not resolve module Declarations.Declarations (/home/runner/work/codeql/codeql/javascript/ql/lib/definitions.qll:8,16-41)
ERROR: Could not resolve predicate Decl/0 (/home/runner/work/codeql/codeql/javascript/ql/lib/definitions.qll:57,47-51)
ERROR: Could not resolve predicate firstRefInTopLevel/3 (/home/runner/work/codeql/codeql/javascript/ql/lib/definitions.qll:57,10-[28](https://github.com/github/codeql/runs/7102304396?check_suite_focus=true#step:4:29))

@aibaars
Copy link
Contributor

aibaars commented Jun 29, 2022

For JavaScript we probably need to move https://github.com/github/codeql/blob/main/javascript/ql/src/Declarations/Declarations.qll to lib as well. @asgerf what do you think?

@erik-krogh
Copy link
Contributor

erik-krogh commented Jun 29, 2022

For JavaScript we probably need to move https://github.com/github/codeql/blob/main/javascript/ql/src/Declarations/Declarations.qll to lib as well. @asgerf what do you think?

It seems very reasonable to move Declarations.qll to lib 👍

@aeisenberg
Copy link
Contributor Author

aeisenberg commented Jun 29, 2022

Yes, looks like I need to move Declarations.qll as well. That should fix the qldoc job failure.

@aeisenberg
Move contextual queries from src to lib
a3f4d1b 
With this change, users are now able to run View AST command in
vscode within vscode workspaces that do not include the core libraries.
The relevant core library only needs to be installed in the package
cache.
@aeisenberg aeisenberg force-pushed the aeisenberg/move-contextual-queries branch from e05d6aa to a3f4d1b Compare June 29, 2022 14:51
@aeisenberg aeisenberg added the no-change-note-required This PR does not need a change note label Jun 29, 2022
@jketema
Copy link
Contributor

jketema commented Jun 29, 2022

Does this require a change note?

I'm a bit split on this. I guess the questions is: how likely is it that someone (besides us) depends on the current location? If it's somewhat likely (I sincerely hope not), it might be good to add a change note that we moved them.

I am getting a failing documentation check. None of these predicates were documented before. Is there some way to skip this check for the failing predicates?

It seems that these can be fixed relatively straightforwardly, so I would just fix them.

@aeisenberg aeisenberg removed the no-change-note-required This PR does not need a change note label Jun 29, 2022
@aeisenberg
Copy link
Contributor Author

aeisenberg commented Jun 29, 2022

I added the qldocs and changenotes.

@aeisenberg aeisenberg closed this Jun 29, 2022
@aeisenberg aeisenberg reopened this Jun 29, 2022
jketema
jketema previously approved these changes Jun 29, 2022
View reviewed changes
@aeisenberg aeisenberg merged commit fbeecd6 into main Jun 29, 2022
@aeisenberg aeisenberg deleted the aeisenberg/move-contextual-queries branch June 29, 2022 18:44
aeisenberg added a commit to aeisenberg/codeql that referenced this pull request Jul 15, 2022
@aeisenberg
Ensure the <lang>-lgtm-full.qls suites include definitions.ql queries
64876db 
In github#9744, these queries were moved from `src` to `lib`. Now they are
no longer part of the <lang>-full-suite.qls, so these suites need to
be expanded to include them.
This was referenced Jul 15, 2022
Closed
Merged
michaelnebel
michaelnebel reviewed Jul 18, 2022
View reviewed changes
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C# Looks plausible to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelnebel michaelnebel michaelnebel left review comments

@jketema jketema jketema approved these changes

Assignees

No one assigned

Labels

C# C++ documentation Java JS Python Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aeisenberg @jketema @aibaars @erik-krogh @michaelnebel
X Tutup