-
-
Notifications
You must be signed in to change notification settings - Fork 386
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
Update deps #2350
Conversation
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
3127d66
to
84e4fe4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments
arduino/sketch/profiles.go
Outdated
ProfilesRaw yaml.Node `yaml:"profiles"` | ||
Profiles []*Profile `yaml:"-"` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 306d7ce
arduino/sketch/profiles.go
Outdated
@@ -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() |
There was a problem hiding this comment.
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:
arduino-cli/arduino/sketch/yaml.go
Line 77 in f561da0
if err := yaml.Unmarshal(dstYaml, &dst); err != nil { |
e8cf3cf
to
e8a879b
Compare
e8a879b
to
4220d18
Compare
4220d18
to
ca55b4d
Compare
Please check if the PR fulfills these requirements
See how to contribute
before creating one)
our contributing guidelines
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:
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