Skip to content

Data Flow: Deprecate old data flow api. #14983

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 6 commits into from
Dec 8, 2023

Conversation

aschackmull
Copy link
Contributor

No description provided.

@jketema
Copy link
Contributor

jketema commented Dec 2, 2023

Fix for the coding standards tests: github/codeql-coding-standards#468

@@ -14,6 +14,8 @@ import FlowStateString
private import codeql.util.Unit
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should deprecate the FlowStateString module in DataFlowImplCommon.qll as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but I figured that was slightly more likely to break something, so I initially left it out, in order to hopefully merge this faster. Then deprecation of FlowStateString can follow in another PR.

@aschackmull aschackmull force-pushed the dataflow/deprecate-old-api branch from 8a16b85 to 46531e6 Compare December 4, 2023 11:37
RasmusWL and others added 2 commits December 4, 2023 14:49
I had missed these originally, since I had just fixed the ones that were
highlighted in the actions logs, thinking they had covered everything :(
@aschackmull aschackmull marked this pull request as ready for review December 8, 2023 12:31
@aschackmull aschackmull requested review from a team as code owners December 8, 2023 12:31
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.

LGTM!

@jketema
Copy link
Contributor

jketema commented Dec 8, 2023

This probably still needs some change notes?

@aschackmull
Copy link
Contributor Author

Merging. I've put the change note in a separate PR to avoid re-running all the CI: #15046

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