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 "config get" command to print settings values #2307

Merged
merged 10 commits into from
Feb 14, 2024

Conversation

ardnew
Copy link
Contributor

@ardnew ardnew commented Sep 10, 2023

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

Feature addition

What is the current behavior?

There are config commands to add/set individual settings, but retrieving settings requires the user to dump all settings and parse the output with either JSON/YAML to retrieve an individual.

What is the new behavior?

With the addition of a config get command, the user can retrieve individual configuration settings without having to parse JSON/YAML.

Does this PR introduce a breaking change, and is titled accordingly?

Not a breaking change

Other information

None

@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself topic: CLI Related to the command line interface labels Sep 10, 2023
@MatteoPologruto
Copy link
Contributor

Hello @ardnew! Thanks for your contribution.
Is this PR ready to be reviewed? If so, can you please rebase your branch onto arduino:master to include the latest commits and resolve any conflicts?
If you have any questions, please feel free to ask!

@umbynos umbynos removed the request for review from MatteoPologruto January 23, 2024 11:09
@ardnew
Copy link
Contributor Author

ardnew commented Jan 24, 2024

@MatteoPologruto Will try to rebase this PR very soon to fix the conflicts.

Also, there were 2 items in the PR template that had not been completed (see below), which is why this was originally posted as a draft. Not sure if updating the docs is essential in this case or if I can submit the PR without it (after resolving conflicts)?

Checklist item Status
Docs have been added / updated (for bug fixes / features) Not sure which sections are mandatory to update. Is this at my discretion?
configuration.schema.json updated if new parameters are added. Will not complete (no changes required)

@MatteoPologruto
Copy link
Contributor

@ardnew don't worry, updating the docs is not needed in this case. The items you checked are the mandatory ones for this kind of contribution. I was asking if this was still a draft because I did not know if you were planning to further modify this PR. Since this is not the case, I will proceed with my review.

internal/cli/config/get.go Outdated Show resolved Hide resolved
@ardnew ardnew marked this pull request as ready for review January 25, 2024 22:52
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

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

Comparison is base (2b2394e) 68.95% compared to head (2942031) 68.97%.
Report is 3 commits behind head on master.

Files Patch % Lines
internal/cli/config/get.go 68.42% 10 Missing and 2 partials ⚠️
commands/upload/upload.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2307      +/-   ##
==========================================
+ Coverage   68.95%   68.97%   +0.01%     
==========================================
  Files         204      204              
  Lines       20535    20449      -86     
==========================================
- Hits        14160    14104      -56     
+ Misses       5220     5193      -27     
+ Partials     1155     1152       -3     
Flag Coverage Δ
unit 68.97% <67.50%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

internal/cli/config/get.go Outdated Show resolved Hide resolved
internal/cli/config/get.go Outdated Show resolved Hide resolved
internal/cli/config/get.go Outdated Show resolved Hide resolved
@MatteoPologruto
Copy link
Contributor

@ardnew code LGTM 🎉
You only need to update the tests according to the changes you made. After that, I'd say we are ready to merge!

Copy link
Contributor

@MatteoPologruto MatteoPologruto left a comment

Choose a reason for hiding this comment

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

Great job! Thanks for your contribution @ardnew 🎉

@MatteoPologruto MatteoPologruto merged commit 8314f87 into arduino:master Feb 14, 2024
98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: CLI Related to the command line interface topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants