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

Add --mir-version command line option. solves MirServer/mir#3058 #3130

Merged
merged 7 commits into from Nov 17, 2023

Conversation

pillowtrucker
Copy link
Contributor

Tested with miral-shell built from within this repo and with my own compositor linking against this version.
I also looked in unit tests to see if I could add it there, but the existing test for options uses test data rather than real responses from built artifacts. Please let me know if there's another one that I've missed that would be suitable for this.

Copy link
Collaborator

@Saviq Saviq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pillowtrucker thanks for this!

Some suggestions inline.

src/platform/options/default_configuration.cpp Outdated Show resolved Hide resolved
src/platform/options/default_configuration.cpp Outdated Show resolved Hide resolved
src/platform/options/default_configuration.cpp Outdated Show resolved Hide resolved
Co-authored-by: Michał Sawicz <michal@sawicz.net>
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (f13aabd) 77.83% compared to head (ce8e6a0) 77.82%.
Report is 8 commits behind head on main.

Files Patch % Lines
src/platform/options/default_configuration.cpp 42.85% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3130      +/-   ##
==========================================
- Coverage   77.83%   77.82%   -0.01%     
==========================================
  Files        1064     1074      +10     
  Lines       74416    74867     +451     
==========================================
+ Hits        57923    58267     +344     
- Misses      16493    16600     +107     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pillowtrucker
Copy link
Contributor Author

Replied in the remaining thread. I'm also not sure if there was a good reason why --help was also throwing an exception, so I've flattened it in the same way. The 0 return code makes my shell happier.

@pillowtrucker
Copy link
Contributor Author

pushed new rebased version ignoring the stray commits from before, added a comment about the thrown exceptions and tried to DRY it a little bit

Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd leave blank lines around the if block to give the reader clearer code "blocks". But not everyone on the project feels the same

Copy link
Collaborator

@Saviq Saviq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Saviq Saviq added this pull request to the merge queue Nov 17, 2023
Merged via the queue into canonical:main with commit c1d2670 Nov 17, 2023
22 of 24 checks passed
@pillowtrucker pillowtrucker deleted the warmup2 branch November 17, 2023 15:40
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.

None yet

3 participants