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 version check command #190

Merged
merged 8 commits into from
Jan 16, 2024
Merged

Add version check command #190

merged 8 commits into from
Jan 16, 2024

Conversation

0x48piraj
Copy link
Contributor

Related Issue(s) nasa/fprime#2407
Has Unit Tests (y/n) n
Documentation Included (y/n) y

Change Description

Adds a version check command for

  • Python version
  • Pip version
  • CMake version and
  • Toolchain version info

Rationale

Help debug things

Future Work

Unit tests preferably.

@thomas-bc thomas-bc self-requested a review January 10, 2024 02:34
@0x48piraj
Copy link
Contributor Author

I also added a skeleton unit test for commands (as asked above in the PR), it's pretty empty as of now but I would be happy to write tests for special-parser commands.

Copy link
Contributor

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you very much!

Looks good to me - just requesting a couple of very minor changes on formatting etc...

Also make sure to take a look at the spellings that were flagged by CI and to add them in https://github.com/0x48piraj/fprime-tools/blob/fd5add0f8781825ea90bfccdcd8d3fb1872d36b2/.github/actions/spelling/expect.txt if they are false positives.

src/fprime/util/commands.py Outdated Show resolved Hide resolved
src/fprime/util/cli.py Show resolved Hide resolved
src/fprime/util/commands.py Outdated Show resolved Hide resolved
src/fprime/util/cli.py Outdated Show resolved Hide resolved
@thomas-bc
Copy link
Contributor

Looks like you also need to run the formatter -

pip install black
black ./path/to/file_to_format.py

I figured you might want to take this across the finish line yourself, but let me know if you want me to help make some changes.

@0x48piraj
Copy link
Contributor Author

All the requested changes were made including the unused import one and it works as intended in local settings.

@thomas-bc
Copy link
Contributor

Thanks! Note to myself: add OS and processor architecture information as well

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

A few minor nits, but this is great work!

test/fprime/util/commands_unit_test.py Show resolved Hide resolved
src/fprime/util/commands.py Outdated Show resolved Hide resolved
src/fprime/util/commands.py Show resolved Hide resolved
@0x48piraj
Copy link
Contributor Author

Rather than using sys, I utilized the platform library for retrieving all system info.

@0x48piraj
Copy link
Contributor Author

Any updates on this @thomas-bc?

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Looks good!

@thomas-bc
Copy link
Contributor

Apologies for the delay, I was out for a bit. LGTM! Thank you very much for this, we want to add this command to our bug_report.md issue template over on https://github.com/nasa/fprime. Feel free to make the PR yourself there if you want to! e.g. add a section/checkmark

[] Please paste the output of fprime-util version-check:

@thomas-bc thomas-bc merged commit 6bf10c5 into nasa:devel Jan 16, 2024
29 checks passed
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.

3 participants