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.

Sign up for GitHub

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

Yarn Berry refactoring and minor improvements #775

Merged
merged 6 commits into from
Jan 10, 2025
Merged

Yarn Berry refactoring and minor improvements #775

merged 6 commits into from
Jan 10, 2025

Conversation

brunoapimentel
Copy link
Contributor

  • Turns the YarnRc and PackageJson project classes into UserDict subclasses (to avoid mapping all necessary attributes)
  • Avoids parsing the version from PackageJson twice when setting the .yarnrc.yml configuration

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

a-ovchinnikov
a-ovchinnikov approved these changes Jan 2, 2025
View reviewed changes
Copy link
Collaborator

@a-ovchinnikov a-ovchinnikov left a comment

Choose a reason for hiding this comment

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

LGTM with a minor suggestion.

cachi2/core/package_managers/yarn/project.py Outdated Show resolved Hide resolved
eskultety
eskultety reviewed Jan 3, 2025
View reviewed changes
Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

First of all, thank you for this effort which I started in the past but failed to finish (https://github.com/eskultety/cachi2/commits/yarn-misc/). I left a couple of design comments, one major, the rest is minor.

cachi2/core/package_managers/yarn/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn/main.py Show resolved Hide resolved
cachi2/core/package_managers/yarn/project.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn/project.py Outdated Show resolved Hide resolved
@brunoapimentel brunoapimentel force-pushed the yarn-refactor branch 2 times, most recently from 15cac3c to 1301bfa Compare January 3, 2025 20:37
eskultety
eskultety reviewed Jan 8, 2025
View reviewed changes
cachi2/core/package_managers/yarn/main.py Show resolved Hide resolved
cachi2/core/package_managers/yarn/project.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn/project.py Outdated Show resolved Hide resolved
tests/unit/package_managers/yarn/test_project.py Show resolved Hide resolved
cachi2/core/package_managers/yarn/main.py Show resolved Hide resolved
cachi2/core/package_managers/yarn/main.py Show resolved Hide resolved
cachi2/core/package_managers/yarn/project.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn/project.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn/main.py Outdated Show resolved Hide resolved
@brunoapimentel brunoapimentel force-pushed the yarn-refactor branch 5 times, most recently from f1f8b50 to 87638b4 Compare January 9, 2025 21:08
@eskultety
Copy link
Member

So the integration test is failing because lockfileFilename doesn't exist as a setting in v4 [1][2] which is one of the things I hinted on during my review. This issue only got visible when the change to merge the defaults inside the YarnRc constructor was made and .yarnrc got written to the disk. While we could revert that change and go back to __getitem__ I think this shows us that we need to be more diligent about new version support for the backends and in order to make our lives easier when it comes to maintenance and make all of this clear in the code we should guard any extra options or those which were dropped guarded by a version conditional wherever needed, in this case - the YarnRc constructor.

[1] https://yarnpkg.com/configuration/yarnrc
[2] https://v3.yarnpkg.com/configuration/yarnrc/#lockfileFilename

@brunoapimentel
yarn: Transform the YarnRc to a UserDict subclass
ed3dde2 
When the class was initially envisioned, we assumed we would need to map
just a few attributes from all the possible keys for yarnrc. As the
implementation progressed, this proved to be a shortcoming.

This patch changes the class to be a UserDict subclass, and effectively
abandons the effort to control the keys that can be used. Default values
for each attribute are also no longer mapped, so the class will only
contain what is explicitly written in the yarnrc file.

With these changes, we already have the benefit of a greatly simplified
class, since we are removing all the properties that mapped to every
single necessary yarnrc option. Any option that we might eventually need
can now be used with no or little changes to YarnRc.

Signed-off-by: Bruno Pimentel <bpimente@redhat.com>
@brunoapimentel
yarn: Convert PackageJson class to a UserDict subclass
89cfa4e 
This change is meant to keep both the YarnRc and PackageJson classes
with similar implementations.

Signed-off-by: Bruno Pimentel <bpimente@redhat.com>
@brunoapimentel
yarn: simplify yarnrc parsing unit test
1eb537b 
With the change of the YarnRc class to a UserDict subclass, the test
that covers parsing a valid .yarnrc.yml file can be greatly simplified.

Signed-off-by: Bruno Pimentel <bpimente@redhat.com>
@brunoapimentel
yarn: refactor code to avoid using type ignore to silence mypy
1647477 
Signed-off-by: Bruno Pimentel <bpimente@redhat.com>
@brunoapimentel
yarn: make _configure_yarn_version return a semver.Version
abdbc3d 
This makes it possible to avoid parsing the version from package.json
a second time when setting the .yarnrc.yaml configuration (there's a
configuration key that is dependent on the Yarn version).

Signed-off-by: Bruno Pimentel <bpimente@redhat.com>
@brunoapimentel
yarn: remove unused registry_server_for_scope function
2c4ffa9 
There's no need for Cachi2 to directly access the contents of the
"npmRegistryServer" yarn_rc variable, since Yarn itself will read it
when we execute "yarn install". The code refering to it is a leftover of
the development phase.

Signed-off-by: Bruno Pimentel <bpimente@redhat.com>
@brunoapimentel
Copy link
Contributor Author

So the integration test is failing because lockfileFilename doesn't exist as a setting in v4 [1][2] which is one of the things I hinted on during my review.

:shakes_fist: yarn

Well, this is tricky, because we'd still need to have the "default" yarn.lock string regardless of it being dropped from the yarnrc settings so that we can actually find the file.

Since we only rely on three config keys, I changed the approach to completely drop the defaults, and treat the cases where we need them during the code (each key is only accessed once, anyways).

@eskultety
Copy link
Member

Since we only rely on three config keys, I changed the approach to completely drop the defaults, and treat the cases where we need them during the code (each key is only accessed once, anyways).

That's fair and should suffice for now. We can improve upon that in the future.

@brunoapimentel brunoapimentel added this pull request to the merge queue Jan 10, 2025
Merged via the queue into containerbuildsystem:main with commit 7557f2c Jan 10, 2025
15 checks passed
@brunoapimentel brunoapimentel deleted the yarn-refactor branch January 10, 2025 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-ovchinnikov a-ovchinnikov a-ovchinnikov approved these changes

@slimreaper35 slimreaper35 slimreaper35 approved these changes

@eskultety eskultety eskultety approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brunoapimentel @eskultety @a-ovchinnikov @slimreaper35