search

thestr4ng3r / chiaki

Moved to https://git.sr.ht/~thestr4ng3r/chiaki - Free and Open Source PS4 Remote Play Client
https://git.sr.ht/~thestr4ng3r/chiaki
2.17k stars 373 forks source link

Re-add AppStream #381

Closed fenio closed 3 years ago

fenio commented 3 years ago

I can't add reviewers myself. @AsciiWolf can you add ximion here to allow him to confirm that such version is fine? Also can you confirm you're good with such version?

AsciiWolf commented 3 years ago

LGTM, except the one typo in url tag. Just please squash the two commits into one and rename it to something better if possible. ;-)

AsciiWolf commented 3 years ago

/cc @ximion

fenio commented 3 years ago

I'm not sure how to squash two commits into one. Do I have to repeat everything? I don't see any option like that in GH.

AsciiWolf commented 3 years ago

You can use git rebase -i HEAD~2 for this. For example, see this guide. ;-)

ximion commented 3 years ago

Looks good to me! But please validate the metainfo file to find typos and other issues:

~$ appstreamcli validate /tmp/re.chiaki.Chiaki.appdata.xml
W: re.chiaki.Chiaki:12: url-invalid-type
E: re.chiaki.Chiaki:12: type-property-required url (https://github.com/thestr4ng3r/chiaki/blob/master/README.md#usage)

Validation failed: errors: 1, warnings: 1, pedantic: 2

(Personally I also would like more descriptive commit messages, but that's up to the project maintainer. Btw, you can leave out the update_contact field if you do not want to share your email address in cleartext - that field is rarely used, if at all, nowadays)

AsciiWolf commented 3 years ago

The warning from appstreamcli is the typo that I already pointed out. :-)

ximion commented 3 years ago

Yes, but it could have been found immediately if the validator was used ;-) You could also run this validation command automatically with the --no-net flag as a cmake test, to make sure the file is always valid. But that's a bonus, and completely not essential.

AsciiWolf commented 3 years ago

Yeah, that's true. :-) I personally always use AppStream validator. By the way, regarding the update_contact e-mail, that is an address provided by @thestr4ng3r.

fenio commented 3 years ago

Guys I used validator and that typo is fixed in 2nd commit which is also part of this PR. I'm probably too dumb to do PR correctly as GH still shows file with typo. Will try to fix it later today.

fenio commented 3 years ago

Ok from my point of view this is something that can be merged. I know that comments of commits are pretty bad but I don't care. Changing them is not recommended as far as could read about it. I really wanted to simply reintegrate that stuff back. If all I prepared is not acceptable then well.. someone more familiar with git / github needs to do it.

AsciiWolf commented 3 years ago

The commits can be squashed during PR merge.

thestr4ng3r commented 3 years ago

@AsciiWolf would you like to approve the pr too?

thestr4ng3r commented 3 years ago

Thanks for the collaborative effort!