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

Prevent duplicate entries in board_manager.additional_urls #1902

Merged
merged 5 commits into from
Oct 5, 2022
Merged

Prevent duplicate entries in board_manager.additional_urls #1902

merged 5 commits into from
Oct 5, 2022

Conversation

henrygab
Copy link
Contributor

@henrygab henrygab commented Oct 2, 2022

Fixes #1437.

Please check if the PR fulfills these requirements

  • 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)
  • What kind of change does this PR introduce?

This change ensures that duplicate values are not introduced into configuration, such as for board_manager.additional_urls

  • What is the current behavior?

The following will result in two identical entries in the config:

arduino-cli config add board_manager.additional_urls http://drazzy.com/package_drazzy.com_index.json
arduino-cli config add board_manager.additional_urls http://drazzy.com/package_drazzy.com_index.json
  • What is the new behavior?

Entries that accept multiple values are ensured to be unique. Care was taken to retain the order of entries. No error is generated due to duplicate entries (no error was previously generated ... this just prevents useless growth of this array).

This is not a breaking change.

  • Other information:

Fixes #1437

@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

Base: 36.71% // Head: 36.68% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (dd85466) compared to base (e404a3b).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1902      +/-   ##
==========================================
- Coverage   36.71%   36.68%   -0.04%     
==========================================
  Files         231      231              
  Lines       19723    19740      +17     
==========================================
  Hits         7242     7242              
- Misses      11652    11669      +17     
  Partials      829      829              
Flag Coverage Δ
unit 36.68% <0.00%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
cli/config/add.go 0.00% <0.00%> (ø)
cli/config/set.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@henrygab henrygab marked this pull request as ready for review October 3, 2022 00:05
@henrygab henrygab changed the title Prevent duplicate board_manager.additional_urls Prevent duplicate entries in board_manager.additional_urls Oct 4, 2022
@henrygab
Copy link
Contributor Author

henrygab commented Oct 5, 2022

The tests that exercise the code paths ran during CI. Not sure why codecov report isn't showing coverage. Some bug in codecov?

@per1234
Copy link
Contributor

per1234 commented Oct 5, 2022

Hi @henrygab. Thanks so much for your pull request and also for your attention to code coverage!

Unfortunately the Python-based integration tests are not considered in the coverage data. This data only describes the coverage provided by the Go-based unit tests like the ones you see here:

https://github.com/arduino/arduino-cli/blob/master/arduino/builder/sketch_test.go

If you are interested in this subject, there is additional information in this proposal for how we might eventually also determine the coverage provided by the integration tests: #1829

@cmaglie cmaglie merged commit d27ba3d into arduino:master Oct 5, 2022
@cmaglie
Copy link
Member

cmaglie commented Oct 5, 2022

Thank you @henrygab!

@cmaglie cmaglie added priority: low Resolution is a low priority type: imperfection Perceived defect in any part of project topic: CLI Related to the command line interface criticality: medium Of moderate impact labels Oct 5, 2022
@cmaglie cmaglie self-assigned this Oct 5, 2022
@cmaglie
Copy link
Member

cmaglie commented Oct 5, 2022

Unfortunately the Python-based integration tests are not considered in the coverage data. This data only describes the coverage provided by the Go-based unit tests like the ones you see here:

We are also migrating the python test into an equivalent version using golang test capabilities, you can find them inside the directory internal/integrationtests. The python test infrastructure will be eventually removed in the near future and the README updated accordingly.

@henrygab henrygab deleted the uniquify_config_arrays branch October 5, 2022 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
criticality: medium Of moderate impact priority: low Resolution is a low priority topic: CLI Related to the command line interface type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command config add for additional URLs does not check presence of URL in board_manager tree
3 participants