[skip changelog] Use appropriate name for field that stores library folder name #1900

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

Jump to bottom

Merged

per1234 merged 1 commit into arduino:master from per1234:library-folder-field-name

Oct 4, 2022

Merged

[skip changelog] Use appropriate name for field that stores library folder name #1900

per1234 merged 1 commit into arduino:master from per1234:library-folder-field-name

Oct 4, 2022

Conversation

per1234

Copy link

Contributor

@per1234 per1234 commented Oct 2, 2022

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?

Bug fix.

What is the current behavior?

Each Arduino library has a name, which is used as its sole identifier by the user in arduino-cli lib commands, and by Arduino CLI when referring to the library in messages displayed to the user.

The name is defined by:

"1.5 format" libraries: name field in the library.properties metadata file
"1.0 format" (AKA "legacy") libraries: installation folder name

The name is resolved when loading the library and stored in the Name field of the github.com/arduino/arduino-cli/arduino/libraries.Library struct.

The name of the library's installation folder is used by Arduino CLI in several other ways, most notably for determining "folder name priority" for use in library dependency resolution. For this reason, the folder name is also stored in the struct when loading the library.

Arduino CLI and arduino-builder have been plagued by problems caused by the inappropriate use of this folder name as the identifier for the library instead of the sole correct identifier (which is only the folder name in the case of "1.0 format" libraries (#932). The design of the github.com/arduino/arduino-cli/arduino/libraries.Library struct may have been a contributing factor in those bugs, since at the time of their occurrence the folder name was stored in the Name field, the metadata-defined name in a RealName. In addition to the fact that no one field could be used as a source of the name in all cases, I suspect the ambiguous field names themselves caused confusion to developers.

This situation was improved by providing the library identifier via a single field for all library formats (#1878). The name provided by this field is the "canonical" name of the library. Inexplicably, at that time the field containing the folder name was renamed CanonicalName. The string contained by this field is in no way a "canonical" name for the library, so the field name is bound to cause more of the very bugs and confusion the redesign of the struct was intended to prevent.

What is the new behavior?

Change the inappropriate github.com/arduino/arduino-cli/arduino/libraries.Library.CanonicalName field name to the accurate DirName.

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

Although the redesign of the github.com/arduino/arduino-cli/arduino/libraries.Library struct is a breaking change, this PR is only a patch on the previous PR that introduced the breaking change. Since there hasn't been a release since the time of that change, this PR does not introduce any new breaking changes.

@per1234


 [skip changelog] Use appropriate name for field that stores library f...

cb7b0fb

...older name
Each Arduino library has a name, which is used as its sole identifier by the user in `arduino-cli lib` commands, and by
Arduino CLI when referring to the library in messages displayed to the user.
The name is defined by:
- "1.5 format" libraries: `name` field in the library.properties metadata file
- "1.0 format" (AKA "legacy") libraries: installation folder name
The name is resolved when loading the library and stored in the `Name` field of the
`github.com/arduino/arduino-cli/arduino/libraries.Library` struct.
The name of the library's installation folder is used by Arduino CLI in several other ways, most notably for determining
"folder name priority" for use in library dependency resolution. For this reason, the folder name is also stored in the
struct when loading the library.
Arduino CLI and arduino-builder have been plagued by problems caused by the inappropriate use of this folder name as the
identifier for the library instead of the sole correct identifier (which is only the folder name in the case of
"1.0 format" libraries. The design of the `github.com/arduino/arduino-cli/arduino/libraries.Library` struct may have
been a contributing factor in those bugs, since at the time of their occurrence the folder name was stored in the `Name`
field, the metadata-defined name in a `RealName`. In addition to the fact that no one field could be used as a source of
the name in all cases, I suspect the ambiguous field names themselves caused confusion to developers.
This situation was improved by providing the library identifier via a single field for all library formats. The name
provided by this field is the "canonical" name of the library. Inexplicably, at that time the field containing the
folder name was renamed "CanonicalName". The string contained by this field is in no way a "canonical" name for the
library, so the field name is bound to cause more of the same bugs and confusion the redesign of the struct was intended
to prevent.
The inappropriately named `github.com/arduino/arduino-cli/arduino/libraries.Library.CanonicalName` field is hereby
renamed to the accurate `DirName`.

@per1234 per1234 added topic: code type: imperfection labels

Oct 2, 2022

@per1234 per1234 requested a review from cmaglie

October 2, 2022 08:01

@per1234 per1234 self-assigned this

Oct 2, 2022

@codecov

Copy link

codecov bot commented Oct 2, 2022

Codecov Report

Base: 36.71% // Head: 36.76% // Increases project coverage by +0.04% 🎉

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

Additional details and impacted files

@@ Coverage Diff @@
## master #1900 +/- ##
==========================================
+ Coverage 36.71% 36.76% +0.04% 
==========================================
 Files 231 231 
 Lines 19723 19723 
==========================================
+ Hits 7242 7251 +9 
+ Misses 11652 11645 -7 
+ Partials 829 827 -2

Flag	Coverage Δ
unit	`36.76% <92.30%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
arduino/libraries/libraries.go	`43.47% <ø> (ø)`
legacy/builder/create_cmake_rule.go	`8.80% <0.00%> (ø)`
arduino/libraries/librariesresolver/cpp.go	`91.27% <100.00%> (ø)`
arduino/libraries/loader.go	`70.45% <100.00%> (ø)`
legacy/builder/phases/libraries_builder.go	`32.45% <100.00%> (ø)`
legacy/builder/types/types.go	`62.50% <100.00%> (ø)`
arduino/cores/packagemanager/package_manager.go	`65.82% <0.00%> (-0.76%)`	⬇️
arduino/monitor/monitor.go	`47.36% <0.00%> (+6.31%)`	⬆️