flathub / com.spotify.Client

https://flathub.org/apps/details/com.spotify.Client
74 stars 35 forks source link

Switch to YAML & move appindicator to shared-modules #300

Open ZVNexus opened 1 day ago

ZVNexus commented 1 day ago

@TingPing i bring more changes

ZVNexus commented 1 day ago

Blocked on https://github.com/flathub/shared-modules/pull/319

flathubbot commented 1 day ago

Started test build 150852

flathubbot commented 1 day ago

Build 150852 failed

Erick555 commented 1 day ago

moving to yaml would break git blame in case someone wants to see historical context of some part of manifest.

it also makes reviewing other changes harder

ZVNexus commented 1 day ago

moving to yaml would break git blame in case someone wants to see historical context of some part of manifest.

What I typically do here is just git follow -- file.json and checkout to the file before it was modified, then you can blame it.

Small price to pay for YAML, JSON really really sucks. Kept forgetting commas, though you could say skill issue.

Erick555 commented 1 day ago

but 99% people looking at it will do it in github, not cli.

yaml is so friendly that it doesn't warn when you break indentation and introduce subtle bugs

ZVNexus commented 1 day ago

but 99% people looking at it will do it in github, not cli.

In that case, you can go to the .yaml file in your browser, find the first commit in it's history, browse files, view history again, and browse files before that commit and then you can blame the JSON

Scales with number of commits obviously though

Erick555 commented 1 day ago

I know but then something taking 1 minute takes 10 min and you may give up.

Erick555 commented 1 day ago

at least you may split converting to separate PR

ZVNexus commented 1 day ago

at least you may split converting to separate PR

Sure, I'll split this up into separate PRs

Erick555 commented 1 day ago

I was thinking about splitting https://github.com/flathub/com.spotify.Client/pull/300/commits/6cd1d1aacb6dcd20b2c35f9cf11be62021496274 and leave the rest combined.

every update results in all users re-downloading whole snap from canonical servers so tiny cosmetic issues aren't worth separate PR.

flathubbot commented 1 day ago

Started test build 150872

flathubbot commented 1 day ago

Build 150872 failed

ZVNexus commented 1 day ago

I was thinking about splitting 6cd1d1a and leave the rest combined.

The idea was to have the YAML commit be dependent on the appindicator inclusion in shared-modules.

every update results in all users re-downloading whole snap from canonical servers so tiny cosmetic issues aren't worth separate PR.

Eh, as long as they're merged relatively close together it should still make it all in the same update for the end user, Flatpaks don't get updated on a machine the moment one is available.

Also the snap is only 180MB, it really doesn't matter.

flathubbot commented 10 hours ago

Started test build 151004

flathubbot commented 10 hours ago

Build 151004 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/134089/com.spotify.Client.flatpakref
ZVNexus commented 7 hours ago

I was thinking about splitting 6cd1d1a and leave the rest combined.

Done: https://github.com/flathub/com.spotify.Client/pull/305