Conversation
fetchPlayStoreAnnouncement() defaulted to owner=theovilardo, repo=PixelPlay — the project's name before the rename to PixelPlayerHQ/PixelPlayer. Works today only because raw.githubusercontent.com still resolves the old path (verified: both URLs return identical content, same etag) — a silent dependency on nobody claiming the old name. Points the defaults at the real repo. Single caller (MainActivity.kt:758) uses no arguments, so this is the only thing that needed to change there. Self-review (code-review skill) found the identical problem in the sibling GitHubContributorService.fetchContributors(), same owner/repo defaults, powering the About screen's contributor list (AboutScreen.kt:202, also called with no arguments) — not named in the original plan, but the same bug. Confirmed empirically: unlike the raw-content case, api.github.com/repos/theovilardo/PixelPlay returns an actual HTTP 301, not a silent 200 — one redirect hop this fix now removes entirely rather than leaving it to keep working by GitHub's grace. Fixed with the same one-line-per-default change. No test added in either file: this predates P.9's genericization of GitHubAnnouncementPropertiesService (fetchRawProperties, separate branch, unmerged) which is what makes that service testable without hitting real network — redoing that refactor here would duplicate P.9's own scope, and GitHubContributorService has no equivalent seam at all. Nothing to assert about a default string value without one. Baseline: 5 pre-existing failures, none new. assembleDebug succeeds.
Author
|
Closing for now — reorganizing how this work is staged. It'll go through our fork first and we'll propose it upstream again, possibly bundled differently, once the larger feature it's part of is further along. Not a rejection, just a process change on our side. |
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.
What
GitHubAnnouncementPropertiesService.fetchPlayStoreAnnouncement()defaultedto
owner=theovilardo, repo=PixelPlay— the project's name before therename to
PixelPlayerHQ/PixelPlayer. One of the small independent fixeslisted in #2813.
It works today only because
raw.githubusercontent.comstill resolves theold path (verified: both URLs return identical content, same etag) — a
silent dependency on nobody claiming the old name.
Change
Points the defaults at the real repo. Single caller (
MainActivity.kt) usesno arguments, so that's the only thing that needed to change there.
A self-review pass (before this PR went up) found the identical problem in
the sibling
GitHubContributorService.fetchContributors(), sameowner/repodefaults, powering the About screen's contributor list(also called with no arguments) — not named in the original plan, but the
same bug. Confirmed empirically that this one is worse than the announcement
case: unlike the raw-content path,
api.github.com/repos/theovilardo/PixelPlayreturns an actual HTTP 301, not a silent 200. Fixed with the same
one-line-per-default change.
Testing
No test added in either file — this predates a separate, unmerged branch
that genericizes
GitHubAnnouncementPropertiesServicein a way that makesit testable without hitting real network; redoing that refactor here would
duplicate that branch's own scope, and
GitHubContributorServicehas noequivalent seam at all. Nothing to assert about a default string value
without one.
assembleDebugsucceeds, full JVM baseline unaffected (5 pre-existingfailures, none new).