The Wayback Machine - https://web.archive.org/web/20220324051445/https://github.com/github/codeql/pull/8476
Skip to content
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

Python/JS/Ruby: Shared concepts scaffolding #8476

Merged
merged 5 commits into from Mar 23, 2022

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Mar 17, 2022

This introduces the base scaffolding from #8307

As mentioned in that PR, the idea is to do separate PRs for each of the concepts we want to share initially, to align on the concept design and query implementation.

@RasmusWL RasmusWL requested review from hmac and esbena Mar 17, 2022
@RasmusWL RasmusWL requested review from as code owners Mar 17, 2022
@RasmusWL RasmusWL added the no-change-note-required label Mar 17, 2022
esbena
esbena previously approved these changes Mar 17, 2022
hmac
hmac previously approved these changes Mar 22, 2022
Copy link
Contributor

@hmac hmac left a comment

Thanks for splitting this out @RasmusWL! I have one minor comment but 👍 otherwise.

/**
* Provides Concepts which are shared across languages.
*
* Each language will have a language specific `Concepts.qll` file that can import the
Copy link
Contributor

@hmac hmac Mar 22, 2022

Choose a reason for hiding this comment

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

Suggested change
* Each language will have a language specific `Concepts.qll` file that can import the
* Each language has a language specific `Concepts.qll` file that can import the

I think this is clearer to the reader, who might wonder if it describes the current state or some future plan that hasn't yet been realised.

Copy link
Member Author

@RasmusWL RasmusWL Mar 22, 2022

Choose a reason for hiding this comment

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

Done in 414764c

Copy link
Contributor

@esbena esbena Mar 22, 2022

Choose a reason for hiding this comment

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

(also, this direct style matches some rule in our doc style guide)

@RasmusWL RasmusWL dismissed stale reviews from hmac and esbena via e50a942 Mar 22, 2022
yoff
yoff approved these changes Mar 23, 2022
Copy link
Contributor

@yoff yoff left a comment

LGTM

hmac
hmac approved these changes Mar 23, 2022
@RasmusWL RasmusWL merged commit bbf60b8 into github:main Mar 23, 2022
37 checks passed
@RasmusWL RasmusWL deleted the shared-concepts-scaffolding branch Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required Python Ruby
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants