-
Notifications
You must be signed in to change notification settings - Fork 30
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
amend branch names to be forked-slug:branch-name #331
Conversation
dana-yaish
commented
Nov 10, 2023
•
edited
edited
- taking get_pull out of is_fork_pr for usability. I don't think it's the responsibility of is_fork_pr to do the request and get the pull's information
- overriding branch in commit's endpoint since that's the only command that actually uses it and sends it in part of the request.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
==========================================
+ Coverage 95.46% 95.48% +0.01%
==========================================
Files 80 80
Lines 2715 2722 +7
==========================================
+ Hits 2592 2599 +7
Misses 123 123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5d9ab1b
to
202ccf0
Compare
202ccf0
to
e313d72
Compare
pull_dict = get_pull(service, decoded_slug, pr) if not token else None | ||
if is_fork_pr(pull_dict): | ||
headers = {} | ||
branch = pull_dict["head"]["slug"] + ":" + branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is good here, just a note for later:
since :
is a legal character in real branch names, i think in the API when we double-check that this PR is from a fork we should set a different field entirely in the database that the UI can check instead of trying to parse it out of the branch name (see ugly screenshot mockup)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but would be a nice UI addition for PRs coming from a fork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i stand corrected, you're right! i should have checked official docs, i just read some page that said git branch names were "very permissive"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still lgtm :)
does the repo automatically dismiss reviews or are you doing that manually?
@matt-codecov nope it's automatic |