Use removesuffix/removeprefix instead of strip for URL and alias parsing#2394
Open
eeshsaxena wants to merge 1 commit into
Open
Use removesuffix/removeprefix instead of strip for URL and alias parsing#2394eeshsaxena wants to merge 1 commit into
eeshsaxena wants to merge 1 commit into
Conversation
Signed-off-by: eeshs <eeshsaxena@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Command.create_pull_requestderives the owner and repo from the export repository URL like this:rstriptakes a set of characters rather than a suffix, so after removing.gitit keeps removing any trailing.,g,iort. Repo names that end in one of those lose characters:The mangled name goes straight into
https://api.github.com/repos/{repo_owner}/{repo_name}/pulls, so the export PR fails with a 404 that reads like a permissions problem. It works today only because the repo currently used ends ina.AdvisoryAlias.urlandAliasV2.urlhave the same idiom:lstrip("NPM-")removes any leadingN,P,Mor-, so it happens to work for the numeric ids that nodejs/security-wg uses, but it would eat the front of anything else. Since thestartswithcheck has already confirmed the prefix,removeprefixsays what is meant and cannot over-strip.Verification: this repo's suite needs Postgres, which I do not have set up here, so I ran the old and new expressions side by side over the URLs above rather than running pytest. The change is pure string handling on three lines and does not touch any query or model behaviour.