Skip to content

Java: rename existing getUrl predicate to getRepositoryUrl #8679

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 2 commits into from
Apr 7, 2022

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Apr 6, 2022

I would like to eventually make all our code follow our own style guide.

That involves changing the location providing predicate getURL to getUrl.
(See providing locations in CodeQL).

However, that change would cause issues in MavenPom.qll where a getUrl() predicate is already used for something else.
With my change the frontend would read the existing getUrl as a predicate that provides a location, and that would cause the MavenPomDependsOnBintray.ql query to break.

I would therefore like to rename it.

@github-actions github-actions bot added the Java label Apr 6, 2022
Copy link
Contributor

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

As the person who originally wrote this query, this LGTM! 👍

Only downside is that this is an API breaking change, so you may need to keep the old API but deprecate it

@erik-krogh
Copy link
Contributor Author

erik-krogh commented Apr 6, 2022

Only downside is that this is an API breaking change, so you may need to keep the old API but deprecate it

I know that is the usual procedure (and I've done it a lot recently).
In this case I would prefer not to, as it could break things if I change the CodeQL frontend to understand getUrl() as a location providing predicate.
Note: the query would have been broken if you had used getURL instead of getUrl, and those are the kind of gotchas I'm trying to prevent in the future.

@erik-krogh erik-krogh marked this pull request as ready for review April 6, 2022 19:19
@erik-krogh erik-krogh requested a review from a team as a code owner April 6, 2022 19:19
@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Apr 6, 2022
@JLLeitschuh
Copy link
Contributor

No complaint from me on the API breaking change, I'm guessing that it isn't going to break anyone. But I don't recommend not including a change note about it. Just my 10 cents.

@erik-krogh erik-krogh removed the no-change-note-required This PR does not need a change note label Apr 6, 2022
@erik-krogh erik-krogh merged commit ef9b6a1 into github:main Apr 7, 2022
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.

3 participants