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

Jump to bottom

JS/Ruby/QL/Python: sync dbscheme fragments #13154

Merged
merged 13 commits into from May 23, 2023
Merged

JS/Ruby/QL/Python: sync dbscheme fragments #13154

merged 13 commits into from May 23, 2023

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented May 12, 2023

No description provided.

@aibaars aibaars marked this pull request as ready for review May 15, 2023 08:01
@aibaars aibaars requested review from a team as code owners May 15, 2023 08:01
@aibaars aibaars changed the title Python: sync dbscheme fragments JS/Ruby/QL/Python: sync dbscheme fragments May 15, 2023
@aibaars aibaars added the no-change-note-required This PR does not need a change note label May 15, 2023
asgerf
asgerf reviewed May 15, 2023
View reviewed changes
javascript/ql/lib/semmlecode.javascript.dbscheme Outdated

/**
* The location of an element that is not an expression or a statement.
Copy link
Contributor

Choose a reason for hiding this comment

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

In JS this contains table the location of statements and expressions as well. Is that not the case in other languages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted, that comment makes little sense for most languages. I meant to delete it but forgot somehow.

@calumgrant calumgrant requested a review from RasmusWL May 15, 2023 09:10
@aibaars aibaars force-pushed the sync-dbscheme-py branch 3 times, most recently from 4ce2def to ce7a71c Compare May 16, 2023 10:32
asgerf
asgerf previously approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

JS 👍 provided that tests and evaluations all look good

RasmusWL
RasmusWL previously approved these changes May 16, 2023
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 changes, which were mostly changing varchar(900) to string it seems.

I assume you've talked with the core team to check that string is the better option for the evaluator? (I don't know this myself, which is why I'm asking)

@aschackmull
Copy link
Contributor

👍 from the Python changes, which were mostly changing varchar(900) to string it seems.

I assume you've talked with the core team to check that string is the better option for the evaluator? (I don't know this myself, which is why I'm asking)

In the dbscheme, string and varchar are synonymous (and the number attached is meaningless). The latter only exists for historical reasons, and you should really use string instead.

@aibaars
Copy link
Contributor Author

aibaars commented May 17, 2023

In the dbscheme, string and varchar are synonymous (and the number attached is meaningless). The latter only exists for historical reasons, and you should really use string instead.

As far as I know the "reprType" is not used at all, it could be anything. Perhaps we should just remove them all at some point.

hmac
hmac approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@hmac hmac left a comment

Choose a reason for hiding this comment

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

Rust changes LGTM 👍

@aibaars aibaars merged commit e33f3a6 into github:main May 23, 2023
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmac hmac hmac approved these changes

@asgerf asgerf asgerf left review comments

@RasmusWL RasmusWL RasmusWL left review comments

@github/codeql-ql-for-ql-reviewers codeql-ql-for-ql-reviewers

@github/codeql-ci-reviewers codeql-ci-reviewers

@alexrford alexrford

@hvitved hvitved

Assignees

@hmac hmac

Labels
JS no-change-note-required This PR does not need a change note Python QL-for-QL Ruby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aibaars @aschackmull @hmac @asgerf @RasmusWL