Skip to content

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.

@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.

@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
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