Skip to content

Python/JS: Make most of the new library private #6200

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.

Sign up for GitHub

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

Jump to bottom
Merged
codeql-ci merged 3 commits into from
Jul 2, 2021
Merged

Python/JS: Make most of the new library private #6200

codeql-ci merged 3 commits into from
Jul 2, 2021

Conversation

@yoff
Copy link
Contributor

@yoff yoff commented Jul 1, 2021

A new library was recently introduced. As much as possible of that should be made private.

@yoff yoff requested review from a team as code owners July 1, 2021 13:42
@yoff yoff mentioned this pull request Jul 1, 2021
Merged
@yoff yoff added the no-change-note-required This PR does not need a change note label Jul 1, 2021
@aschackmull
Copy link
Contributor

aschackmull commented Jul 2, 2021

Much more can be made private.

@yoff
Copy link
Contributor Author

yoff commented Jul 2, 2021

Much more can be made private.

Do you mean in other files or in the new file? I felt I could not hide previously exposed stuff in the javascript codebase, and I want the python files to be identical.

@yoff
Copy link
Contributor Author

yoff commented Jul 2, 2021

Ah, I guess I could make most classes private also..

@yoff
Copy link
Contributor Author

yoff commented Jul 2, 2021

Oh, and I missed all the typed predicates 🤦

yoff added 2 commits July 2, 2021 12:13
@yoff
Python/JS: Make much more private
77c329f
@yoff
Python: make the import of RedosUtil public
6f26426 
This mirrors `SuperlinearBacktracking.qll`
An alternative is to keep it private and import it again
in the query files.
RasmusWL
RasmusWL approved these changes Jul 2, 2021
View reviewed changes
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

👍 from the Python side

@esbena
Copy link
Contributor

esbena commented Jul 2, 2021

A new library was recently introduced.

The library is less new for JavaScript (~4-6 months?). Is this privatization a breaking change for JavaScript? Or did we already cross that bridge when the shared RegExp libraries for Python/JavaScript were introduced?

@aschackmull
Copy link
Contributor

aschackmull commented Jul 2, 2021

Is this privatization a breaking change for JavaScript

I believe this code was recently hidden in a .ql file, so therefore implicitly private. But I haven't checked the details - I'm just drive-by commenting.

@yoff
Copy link
Contributor Author

yoff commented Jul 2, 2021

Is this privatization a breaking change for JavaScript

I believe this code was recently hidden in a .ql file, so therefore implicitly private. But I haven't checked the details - I'm just drive-by commenting.

That is correct, it was inside ReDoS.ql which has to import javascript. It defines the relevant ReDoSConfiguration and was pulled out so that the files can be identical.

esbena
esbena approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

In that case: 👍 from JS

@codeql-ci codeql-ci merged commit 1d56748 into github:main Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RasmusWL RasmusWL RasmusWL approved these changes

@esbena esbena esbena approved these changes

Assignees

No one assigned

Labels

JS no-change-note-required This PR does not need a change note Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yoff @aschackmull @esbena @RasmusWL @codeql-ci