Skip to content

make a shared library of the typo database #10357

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

Merged
merged 3 commits into from
Sep 12, 2022
Merged

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Sep 8, 2022

The typo database was shared between JS and QL-for-QL.
With the new shared libraries we can avoid storing 2 copies of the same file.

@erik-krogh erik-krogh marked this pull request as ready for review September 8, 2022 13:55
@erik-krogh erik-krogh requested review from a team as code owners September 8, 2022 13:55
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Nice. A few small things.

@@ -5,3 +5,5 @@ dbscheme: ql.dbscheme
suites: codeql-suites
defaultSuiteFile: codeql-suites/ql-code-scanning.qls
extractor: ql
dependencies:
codeql/typos: "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, can you be explicit about the dependency here? This will require manually bumping when you change versions.

Suggested change
codeql/typos: "*"
codeql/typos: 0.0.1

We are working on something to make this simpler. See https://github.com/github/codeql-core/issues/2741

@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Sep 9, 2022
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Apart from codeql/typos being a slightly odd name for a package (it doesn't immediately communicate what it's actually about), everything looks good to me. 👍

@erik-krogh erik-krogh merged commit 3384521 into github:main Sep 12, 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 This PR does not need a change note QL-for-QL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants