Skip to content
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

feat(dep): check integrity #1054

Merged
merged 3 commits into from
Mar 18, 2024
Merged

feat(dep): check integrity #1054

merged 3 commits into from
Mar 18, 2024

Conversation

Mte90
Copy link
Contributor

@Mte90 Mte90 commented Mar 14, 2024

Based on #859

This check if innoextract and cabextract are fine.

Check the size is not something that can confirm as you suggested in the ticket, the only one is to run the command and see if crashes.

@sonic2kk
Copy link
Owner

This looks pretty great, thanks! I like this approach a lot, I think it makes sense. I had some comments but ShellCheck summarises them better, I'll leave them as comments on the specific lines themselves.

I realise you had issues with the Bash LSP so it's understandable. As a workaround you can instead download ShellCheck 0.8.0 manually and add it to your path as an alias, that's what I do for ShellCheck 0.8.0. Then from the project folder, you can run shellcheck steamtinkerlaunch to check the script. This will hopefully be unnecessary once a newer ShellCheck version releases, fixing the issue for various scripts.

Apart from that, I was wondering if we should make it a dedicated function, in case we can use this elsewhere, but I don't think we really use many native binaries this way. If needs be in future, we can split it out.

Thanks for your contribution! Once the ShellCheck comments are addressed, I'll merge this :-)

steamtinkerlaunch Outdated Show resolved Hide resolved
steamtinkerlaunch Outdated Show resolved Hide resolved
@Mte90
Copy link
Contributor Author

Mte90 commented Mar 15, 2024

Yeah I am using Scite that doesn't use LSP and doesn't lag with huge files (I use that for mysql dump that often are bigger than this).

@sonic2kk
Copy link
Owner

ShellCheck v0.10.0 is out, and once I add the directive to not use extended analysis, as long as you're using an up-to-date LSP things should be fine.

@sonic2kk
Copy link
Owner

sonic2kk commented Mar 15, 2024

Sorry, I just realised I forgot to mention (and document) that the version should be bumped. There's no exact rule on this, basically just the date for you + 1, and then bump the revision number. So for example you might set v14.0.20240316-1.

This isn't enforced, this is just my own workflow that you might find useful when working on PRs: typically what I do is set the version during development, and in brackets, add the branch name. This means if another PR uses the same date, the version string is still unique. So in this case it might be v14.0.20240316-1 (valid-dependency). Then, once the PR is ready to be merged, I change the version. If you submit a large PR it might be good to do this, and once all feedback is addressed and ready to merge, I can leave a comment to bump the version and then it'll be ready to merge. 🙂

This is used to know when and how to update config files, and they are assigned a version. It's not massively important for afaik any of your PRs but still just good practice (I forget to do it sometimes myself!)

Once this is done, the rest of the changes look good to me, and this will be ready to merge. ShellCheck v0.10.0 is very angry about a lot of this script, but v.0.8.0 gives no warnings after the changes. At a glance though I think v0.10.0 isn't complaining about anything specific to this PR.


Thanks a whole lot! 😄

@Mte90
Copy link
Contributor Author

Mte90 commented Mar 18, 2024

As you asked version bumped.

@sonic2kk
Copy link
Owner

Thanks! Although the string isn't formatted quite right.

Your version string is v14.0.20240316-(valid-dependency).

  • For development, it can be v14.0.20240316-1 (valid-dependency) (note the -1 and the space between (valid-dependency).
  • Before merging, it should be v14.0.20240318-1 (or -2 and so on, depending on the version string on master).

This PR has conflicts because of my mergings, but I'll see if I can rebase this PR myself and merge it if that's ok. 😄 You can also use the diff here as reference for future PRs.


Thanks!

@Mte90
Copy link
Contributor Author

Mte90 commented Mar 18, 2024

Fixed the version and for conflicts, yeah it is better that you do so :-)

@sonic2kk
Copy link
Owner

Heh, I took a quick look at how to contribute to someone else's PR using this gist. I will try my best not to mess anything up on you, but if I do, I can probably just pick your PR into another branch and then squash-merge it (you might end up as a co-author), but you'll be credited correctly in the changelog 😄)

@Mte90
Copy link
Contributor Author

Mte90 commented Mar 18, 2024

To me is perfectly fine :-)

@sonic2kk
Copy link
Owner

I got it in the end I think. Just doing one last ShellCheck run and then I will merge this.

Thank you for your patience, continued discussions and contributions! It is much appreciated.

@sonic2kk sonic2kk merged commit 706d7a1 into sonic2kk:master Mar 18, 2024
@sonic2kk
Copy link
Owner

sonic2kk commented Mar 18, 2024

Changelog has also been updated to include your contribution (might be easiest to see with Ctrl+F for @Mte90): https://github.com/sonic2kk/steamtinkerlaunch/wiki/Changelog - This changelog gets copied and pasted into the release notes, so you will be credited in the same way there too.

Also, it looks like despite the commits in the PR being co-authored (because of the rebase), the squash-merge properly only included you, which I am happy about!

I am very keen on making sure all contributors get properly recognised, which is also why I bring it up here too. Thanks again for all your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Steam Deck Experience Improvements [Community Contributions Wanted]
2 participants