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 deps #2350

Merged
merged 19 commits into from
Oct 13, 2023
Merged

Update deps #2350

merged 19 commits into from
Oct 13, 2023

Conversation

alessio-perugini
Copy link
Contributor

@alessio-perugini alessio-perugini commented Oct 2, 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?

Each commit bumps the version of a dependency.
The critical ones are:

  • viper
  • cobra
  • yaml v3
    We can cherry-pick some of them and put them in the next RC.

What is the current behavior?

What is the new behavior?

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

Other information

@umbynos umbynos added this to the Arduino CLI v0.35.0 milestone Oct 2, 2023
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

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

Comparison is base (3a2f9f1) 62.92% compared to head (b7667b2) 62.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2350      +/-   ##
==========================================
- Coverage   62.92%   62.91%   -0.01%     
==========================================
  Files         203      203              
  Lines       19243    19249       +6     
==========================================
+ Hits        12108    12110       +2     
- Misses       6075     6080       +5     
+ Partials     1060     1059       -1     
Flag Coverage Δ
unit 62.91% <58.33%> (-0.01%) ⬇️

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

Files Coverage Δ
arduino/cores/packagemanager/install_uninstall.go 58.00% <100.00%> (ø)
arduino/libraries/librariesmanager/install.go 65.36% <ø> (ø)
configuration/configuration.go 67.50% <ø> (-0.80%) ⬇️
internal/cli/config/dump.go 71.42% <ø> (ø)
internal/cli/feedback/feedback.go 69.78% <ø> (ø)
arduino/sketch/sketch.go 81.92% <0.00%> (ø)
commands/compile/compile.go 66.78% <50.00%> (ø)
arduino/sketch/profiles.go 73.94% <85.71%> (+4.92%) ⬆️
arduino/cores/status.go 55.85% <0.00%> (-2.09%) ⬇️
...ino/libraries/librariesmanager/librariesmanager.go 70.63% <0.00%> (-2.32%) ⬇️

... and 1 file with indirect coverage changes

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

@alessio-perugini alessio-perugini self-assigned this Oct 2, 2023
@alessio-perugini alessio-perugini marked this pull request as ready for review October 2, 2023 13:58
Copy link
Contributor

@umbynos umbynos left a comment

Choose a reason for hiding this comment

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

Left some comments

Comment on lines 34 to 35
ProfilesRaw yaml.Node `yaml:"profiles"`
Profiles []*Profile `yaml:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change was required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the new yamlv3 package, they removed the MapSlice which would parse the map and respect the order of the key.
So to achieve the same thing we have to use the yaml.Node type and do by hand what essentially the MapSlice was doing. The only downside is that we have to bring the ProfilesRaw just for parsing purposes. I thought that bringing a new struct for parsing purposes and copying the result back to the original struct was a bit too much in this case.

Copy link
Member

Choose a reason for hiding this comment

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

I thought that bringing a new struct for parsing purposes and copying the result back to the original struct was a bit too much in this case.

IMHO it's better to use a support structure, I don't like much this ProfilesRaw field, it's a publicly visible field that is confusing (should I look inside ProfilesRaw or Profiles? Should they be kept synchronized?). I see also that we cannot even make it private because we must unmarshal.
Another advantage is that the support structure may be discarded after being used.

Copy link
Contributor Author

@alessio-perugini alessio-perugini Oct 3, 2023

Choose a reason for hiding this comment

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

Done in 306d7ce

go.mod Show resolved Hide resolved
@@ -260,5 +270,7 @@ func LoadProjectFile(file *paths.Path) (*Project, error) {
if err := yaml.Unmarshal(data, &res); err != nil {
return nil, err
}
res.Profiles = res.getProfiles()
Copy link
Member

Choose a reason for hiding this comment

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

Can't we do this inside the YamlUnmarshal? We may forget to produce the profiles for example we are trying to do another yaml.Unmarshal here:

if err := yaml.Unmarshal(dstYaml, &dst); err != nil {

arduino/sketch/profiles.go Outdated Show resolved Hide resolved
@alessio-perugini alessio-perugini added the topic: code Related to content of the project itself label Oct 3, 2023
@alessio-perugini alessio-perugini merged commit 5e90edf into master Oct 13, 2023
190 checks passed
@alessio-perugini alessio-perugini deleted the update-deps branch October 13, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants