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

Add support for repository autolink references #2016

Merged
merged 4 commits into from Oct 29, 2022

Conversation

theCapypara
Copy link
Contributor

Adds support for repository autolinks. I put this on draft, since the API for it is beta and subject to change.

This will (actually :) ) close #1293.

Copy link
Collaborator

@s-t-e-v-e-n-k s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

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

This is looking like a great start. I have some concerns, one of which I've marked inline, and further, if this API is in beta, does it require a custom Accept header like many other examples in this code base?

github/Repository.py Show resolved Hide resolved
@theCapypara
Copy link
Contributor Author

As far as I can tell a custom Accept header is not needed.

Copy link
Collaborator

@s-t-e-v-e-n-k s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

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

Looking fantastic, just requires tests for the new methods.

@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2021

Codecov Report

Base: 98.81% // Head: 98.80% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (e704d30) compared to base (784a3ef).
Patch coverage: 97.36% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2016      +/-   ##
==========================================
- Coverage   98.81%   98.80%   -0.01%     
==========================================
  Files         109      110       +1     
  Lines       11267    11305      +38     
==========================================
+ Hits        11133    11170      +37     
- Misses        134      135       +1     
Impacted Files Coverage Δ
github/Autolink.py 95.83% <95.83%> (ø)
github/Repository.py 97.18% <100.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@theCapypara
Copy link
Contributor Author

Hi, I looked into creating tests for this, but I'm unsure how to properly get the replay data I need for them.

@s-t-e-v-e-n-k
Copy link
Collaborator

@theCapypara
Copy link
Contributor Author

I can't record replay data since it changes the Repository.setUp replay data to my fork, which makes all the other tests fail. I can't record data using the main repository, since I don't have write access.

@theCapypara
Copy link
Contributor Author

I guess I could change the test case to use my fork only for these methods? But that doesn't really sound like a great solution.

@theCapypara
Copy link
Contributor Author

theCapypara commented Nov 2, 2021

I could also manually change the replay data (replace mentions of my fork with the main repo) I guess, if that's okay.

Edit: That's what I will do for the "Repository" test. For the "Autolink" test case I create I will use my fork, I see that's what others did too.

@theCapypara theCapypara marked this pull request as ready for review November 2, 2021 13:21
@theCapypara
Copy link
Contributor Author

Done. I also undrafted since I'm fairly optimistic this API is stable enough / even if they end up changing something after "Beta" we can just adjust the code again.

@theCapypara
Copy link
Contributor Author

& rebased for good meassure

@theCapypara
Copy link
Contributor Author

@s-t-e-v-e-n-k Do you have an update on the review status?

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@stale stale bot closed this Apr 28, 2022
@theCapypara
Copy link
Contributor Author

@s-t-e-v-e-n-k Can you re-open this?

@megamorf
Copy link

Can we please re-open this ticket and get the code merged. We're in need of the feature and it'd be better to have it as part of PyGithub instead of having to use a custom implementation.

@megamorf
Copy link

@sfdye Can we get this feature into the next release? It would be a waste to throw away all the work that already went into it.

@sfdye sfdye reopened this Oct 28, 2022
@stale stale bot removed the stale label Oct 28, 2022
@sfdye
Copy link
Member

sfdye commented Oct 28, 2022

Re-opened, if someone could resolve the conflicts I will review

@theCapypara
Copy link
Contributor Author

Done.

@megamorf
Copy link

Done.

Looks like one of the CI tests failed. And upon closer inspection it seems that the code needs to be formatted by black to pass the lint check and the import order needs to be updated.

black: https://github.com/PyGithub/PyGithub/actions/runs/3344110825/jobs/5538083016#step:5:39
isort: https://github.com/PyGithub/PyGithub/actions/runs/3344110825/jobs/5538083016#step:5:52

Both should be fixed by running the pre-commit hook on the PR branch.

@sfdye sfdye merged commit 0fadd6b into PyGithub:master Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for "Autolink References" (aka key_links)
5 participants