-
Notifications
You must be signed in to change notification settings - Fork 53
Add CS2Tracker Extension #213
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
Open
MuhammedResulBilkil
wants to merge
2
commits into
SteamClientHomebrew:main
Choose a base branch
from
MuhammedResulBilkil:add-cs2tracker-extension
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+4
−0
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Submodule cs2tracker-extension
added at
9be073
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.
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.
Fair point, and I'd rather answer it than leave it sitting.
The principle is right:
masteris a mutable ref, so the images the store renders after a merge aren't necessarily the images anyone reviewed. Pinning would fix that, and I've checked that it's a clean swap — all the referenced assets already resolve at thev1.0.0tag, so it's a two-line change toplugin.jsonand a submodule bump here, nothing has to move.What makes it awkward rather than automatic is the release pipeline. Releases here are cut by semantic-release, which decides the version number at release time and writes it into
plugin.jsonitself. So a URL namingv1.0.1has to be committed before the tooling has decided the release isv1.0.1, which means hand-holding a value the pipeline otherwise owns, on every release, forever, or the images silently keep pointing at the previous tag.So: I'd rather do this properly at the next release, where I can wire the asset URLs into the same substitution semantic-release already performs on the version field, than bolt a hardcoded tag on now and quietly create a step that gets forgotten. If a maintainer would prefer it pinned before merge, say so and I'll cut a
1.0.1for it — I just don't think a bot-flagged nit is worth a release on its own, and I'd rather ask than assume.Worth noting for whoever picks this up: no other plugin currently in the database sets
thumbnailorsplash_imageat all, so there's no house convention here either way. If the project does want pinned asset refs as a rule, that's a template or docs change rather than something to settle in one submission's thread, and I'm happy to be the first to follow it.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.
Done — this is fixed rather than deferred. Thanks for the flag; it was a fair one.
The submodule now points at
f5e67bf(v1.0.1), whoseplugin.jsonreads:I said earlier I'd rather not hardcode a tag, because that fights
semantic-release— it owns the version field — and adds a release step to forget. That objection turned out to be answerable rather than blocking: the rewrite now happens inside the prepare step that already syncs the version, so the tag in these URLs is derived from the release being cut, not typed by hand.@semantic-release/gitcommitsplugin.jsonduring prepare and semantic-release tags that commit, so the tag the URLs name is the tag they're committed under, andassets/exists there. The apparent chicken-and-egg isn't one. Every future release re-pins itself.The
--checkmode CI already runs gained the matching assertion, so a hand-edit back onto a branch ref now fails the build:$schemais deliberately left on Millennium'smainso editor hints track the schema as it changes; the test suite asserts that exclusion so the rule can't later be widened to "every raw URL" and freeze it by accident.v1.0.1contains no functional change — it exists only to carry this. Both pinned URLs return 200 at the tag.One note for whoever reviews, unchanged from before: no other plugin in the database sets
thumbnailorsplash_imageat all, so there's still no house convention here. If the project wants pinned asset refs as a rule, that's a template or docs change rather than something to settle per-submission — happy to have been the first to follow it either way.