[skip-changelog] Improve the definition of FQBN and explicitly state which characters are allowed

MatteoPologruto commented 5 months ago

Please check if the PR fulfills these requirements

See how to contribute

[x] The PR has no duplicates (please search among the Pull Requests before creating one)
[x] The PR follows our contributing guidelines
[x] Tests for the changes have been added (for bug fixes / features)
[x] 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?

Documentation enhancement

What is the new behavior?

Each field of the fqbn is checked to verify if it contains invalid characters.

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

No

Other information

cmaglie commented 5 months ago

I remember that talking with @cmaglie we wanted to enforce this check in the codebase. Let's wait for his response if we want to tackle that in the same PR.

Yep, it would be nice to start enforcing it. I don't know how much platforms would break, BTW it seems very unlikely (in case we may relax the specification to include some extra char later).

codecov[bot] commented 5 months ago

Codecov Report

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

Comparison is base (51119b2) 68.92% compared to head (b469a22) 68.91%. Report is 1 commits behind head on master.

:exclamation: Current head b469a22 differs from pull request most recent head 0f4bcf2. Consider uploading reports for the commit 0f4bcf2 to get more accurate results

Files	Patch %	Lines
internal/arduino/cores/fqbn.go	50.00%	4 Missing and 2 partials :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #2509 +/- ## ========================================== - Coverage 68.92% 68.91% -0.02% ========================================== Files 204 204 Lines 20452 20464 +12 ========================================== + Hits 14096 14102 +6 - Misses 5207 5211 +4 - Partials 1149 1151 +2 ``` | [Flag](https://app.codecov.io/gh/arduino/arduino-cli/pull/2509/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arduino) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/arduino/arduino-cli/pull/2509/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arduino) | `68.91% <50.00%> (-0.02%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=arduino#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dankeboy36 commented 5 months ago

Thanks for the feature. I see arduin-o:av-r:un-o:a=b=c=d is valid. What config option and value does it imply? Can you confirm that config option a has a value of "b=c=d"?

Update: I see yes

https://github.com/arduino/arduino-cli/blob/aa15421aa1996afbcf958a7d726d91f6da550e1e/internal/arduino/cores/fqbn_test.go#L121

MatteoPologruto commented 4 months ago

Hello @dankeboy36! Yes, I am confirming that arduin-o:av-r:un-o:a=b=c=d is a valid FQBN and the config key a has the value of b=c=d.

arduino / arduino-cli