Skip to content

Make MainClass.get_app return completed GithubApp when slug is given#2543

Merged
EnricoMi merged 2 commits intoPyGithub:masterfrom
EnricoMi:github-app-slug
Jun 8, 2023
Merged

Make MainClass.get_app return completed GithubApp when slug is given#2543
EnricoMi merged 2 commits intoPyGithub:masterfrom
EnricoMi:github-app-slug

Conversation

@EnricoMi
Copy link
Collaborator

@EnricoMi EnricoMi commented Jun 6, 2023 •
edited
Loading

There is no need not to return a non-completed GithubApp at this point. A changing url smells. Fixes #2487.

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.11 🎉

Comparison is base (fc2d0e1) 98.57% compared to head (ec8a857) 98.68%.

❗ Current head ec8a857 differs from pull request most recent head d18ea5d. Consider uploading reports for the commit d18ea5d to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2543      +/-   ##
==========================================
+ Coverage   98.57%   98.68%   +0.11%     
==========================================
  Files         118      117       -1     
  Lines       11980    11832     -148     
==========================================
- Hits        11809    11677     -132     
+ Misses        171      155      -16
Impacted Files Coverage Δ
github/GithubApp.py 98.90% <100.00%> (+0.01%) ⬆️
github/MainClass.py 99.22% <100.00%> (-0.05%) ⬇️

... and 8 files with indirect coverage changes

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

JLLeitschuh
JLLeitschuh approved these changes Jun 7, 2023
View reviewed changes
Copy link
Collaborator

@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.

I don't think I have enough context to fully understand this change and why it was the way it was originally, but the updated version of the code looks good to me!

@EnricoMi
Copy link
Collaborator Author

EnricoMi commented Jun 7, 2023

I have rethought this: EnricoMi@7615867

When a slug is given, an app is fully identified. With this incomplete information, a GithubApp can be returned lazily.

Without a slug, the current app is fetched through /app and a complete object is returned. An incomplete GithubApp without the slug (identifier) is of no use here.

It was the opposite earlier. I think, it makes more sense now. Especially, the app.url does not change any more between before and after completion.

@EnricoMi EnricoMi force-pushed the github-app-slug branch 2 times, most recently from 61e455d to ec8a857 Compare June 7, 2023 18:11
@EnricoMi EnricoMi changed the title Make MainClass.get_app return completed GithubApp Make MainClass.get_app return completed GithubApp when slug is given Jun 8, 2023
@EnricoMi EnricoMi merged commit 84912a6 into PyGithub:master Jun 8, 2023
@EnricoMi EnricoMi deleted the github-app-slug branch June 8, 2023 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JLLeitschuh JLLeitschuh JLLeitschuh approved these changes

@sfdye sfdye Awaiting requested review from sfdye

@s-t-e-v-e-n-k s-t-e-v-e-n-k Awaiting requested review from s-t-e-v-e-n-k

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

GithubApp.slug is None due to no _completeIfNotSet call

3 participants

@EnricoMi @codecov-commenter @JLLeitschuh