Skip to content

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Jun 28, 2022

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

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

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

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

I added the qldocs and changenotes.

jketema
jketema previously approved these changes Jun 29, 2022
@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
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.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants