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

Update behaviour of shpc config get to only show value of key #661

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

marcodelapierre
Copy link
Contributor

Hi @vsoch , here we go, two minimal edits in this PR.

This addresses #658

Note how I have not added a flag to print both value and key, as I have noticed how shpc view get <view> works, and realised that the new behaviour for shpc config get <key> actually exactly matches it, hence it is quite consistent.

Two edits, one per commit:

  1. new behaviour as per subject
  2. when value is not set, changed printed value from is unset to Unset, as I believe it is easier to handle (as a single string without spaces) in case the output is post-processed.

Keen on your feedback as always!

@marcodelapierre
Copy link
Contributor Author

and actually, note how I am now using print instead of logger.info to print, so that the output goes to STDOUT instead of STDERR and can be easily captured within shell pipelines and similar.

@@ -47,8 +47,9 @@ def main(args, parser, extra, subparser):
elif command == "get":
for key in args.params:
value = cli.settings.get(key)
value = "is unset" if value is None else value
logger.info("%s %s" % (key.ljust(30), value))
value = "Unset" if value is None else value
Copy link
Member

Choose a reason for hiding this comment

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

This looks good! Do you have preference for capitalized vs. not? I would expect all lowercase since it's not a sentence. Finally, could you bump the version and changelog?

Copy link
Contributor Author

@marcodelapierre marcodelapierre Oct 10, 2023

Choose a reason for hiding this comment

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

  • updated string above to unset
  • bumped version in version.py
  • updated CHANGELOG

@vsoch vsoch merged commit df6a62c into singularityhub:main Oct 10, 2023
10 checks passed
@vsoch
Copy link
Member

vsoch commented Oct 10, 2023

Thank you @marcodelapierre ! Also this week I'm finally winding down from my turbo mode (second talk is recorded, fingers crossed it's acceptable and I don't need to do it again!) and then I can just go back into lizard programming mindset 24/7 (actually already did today!) flux-framework/flux-operator#208

Gosh sometimes you just need like 2 months mixed with too many papers and presentations to realize how amazing and wonderful programming is (and maybe I already knew that... 😆 )

@marcodelapierre
Copy link
Contributor Author

I am glad you are almost back to your fav work routine!!

I have this second week of leave before I start my new professional adventure..I am relaxing so well now, but also cannot wait to start!!

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.

2 participants