Skip to content

enhancement: Moved all javascript package managers under /javascript for consolidation#1591

Open
mohamedbouchtout wants to merge 4 commits into
hermetoproject:mainfrom
mohamedbouchtout:feature/javascript-package-manager-relocation
Open

enhancement: Moved all javascript package managers under /javascript for consolidation#1591
mohamedbouchtout wants to merge 4 commits into
hermetoproject:mainfrom
mohamedbouchtout:feature/javascript-package-manager-relocation

Conversation

@mohamedbouchtout

@mohamedbouchtout mohamedbouchtout commented Jun 1, 2026

Copy link
Copy Markdown

Summery

Moved the package managers npm, yarn, yarn_classic and metayarn.py file under hermeto/core/package_managers/javascript as mentioned in this issue. Then also updated all imports path for each of the relocated files throughout the project. Also moved their unit tests under the /javascript folder to match.

Details

  1. Moved npm package manager:
    • hermeto/core/package_managers/npm -> hermeto/core/package_managers/javascript/npm
    • hermeto/test/unit/package_managers/npm -> hermeto/test/unit/package_managers/javascript/npm
  2. Moved yarn package manager:
    • hermeto/core/package_managers/yarn -> hermeto/core/package_managers/javascript/yarn
    • hermeto/test/unit/package_managers/yarn -> hermeto/test/unit/package_managers/javascript/yarn
  3. Moved yarn_classic package manager:
    • hermeto/core/package_managers/yarn_classic -> hermeto/core/package_managers/javascript/yarn_classic
    • hermeto/test/unit/package_managers/yarn_classic -> hermeto/test/unit/package_managers/javascript/yarn_classic
  4. Moved metayarn.py file:
    • hermeto/core/package_managers/metayarn.py -> hermeto/core/package_managers/javascript/metayarn.py
    • hermeto/test/unit/package_managers/metayarn.py -> hermeto/test/unit/package_managers/javascript/metayarn.py
  5. Added the /javascript folder in the import paths:
    • In the npm, pnpm, yarn, and yarn_classic package manager
    • In the metayarn.py file
    • In the resolver.py file
    • In all the package manager tests

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request reorganizes the codebase by consolidating JavaScript-related package managers (npm, yarn, yarn_classic, and metayarn) under a new hermeto/core/package_managers/javascript/ directory, updating all relevant imports and mock patches. Feedback on this PR points out that the corresponding unit test files and directories have not been relocated to match this new structure. Moving these tests under tests/unit/package_managers/javascript/ is recommended to maintain a consistent code structure across the codebase.

@@ -5,18 +5,18 @@

from hermeto.core.errors import PackageRejected
from hermeto.core.models.input import Request
from hermeto.core.package_managers.metayarn import fetch_yarn_source
from hermeto.core.package_managers.yarn_classic.main import NotV1Lockfile
from hermeto.core.package_managers.javascript.metayarn import fetch_yarn_source

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The package managers npm, yarn, yarn_classic, and metayarn have been consolidated under hermeto/core/package_managers/javascript/. However, their corresponding unit tests (e.g., tests/unit/package_managers/test_metayarn.py, tests/unit/package_managers/npm/, tests/unit/package_managers/yarn/, and tests/unit/package_managers/yarn_classic/) have not been moved under tests/unit/package_managers/javascript/. To maintain a consistent code structure across the whole codebase, please relocate these test files/directories to match the new package manager structure.

References
  1. Maintain a consistent code structure across the whole code base (link)

@eskultety

Copy link
Copy Markdown
Member

@mohamedbouchtout please make sure every single commit passes both linters and unit tests, i.e. nox.

@mohamedbouchtout mohamedbouchtout force-pushed the feature/javascript-package-manager-relocation branch from fc580a3 to 09f6f69 Compare June 2, 2026 16:04
@slimreaper35

Copy link
Copy Markdown
Member

I believe the first six commits should be combined with the other six.

@mohamedbouchtout

Copy link
Copy Markdown
Author

@slimreaper35 So maybe squash like commit 1, 5, and 9 which would include "Moved npm package manager and it's unit tests under the /javascript folder; updates it's import paths to new location". And do the same with the rest which will bring it down to 4 commit?

@slimreaper35

Copy link
Copy Markdown
Member

@slimreaper35 So maybe squash like commit 1, 5, and 9 which would include "Moved npm package manager and it's unit tests under the /javascript folder; updates it's import paths to new location". And do the same with the rest which will bring it down to 4 commit?

For example, the first commit moves the whole package_managers/npm directory to package_managers/javascript/npm. But the tests still expect the previous package_managers/npm so they fail. It's related to the #1591 (comment).

@mohamedbouchtout mohamedbouchtout force-pushed the feature/javascript-package-manager-relocation branch from 09f6f69 to 34a7c93 Compare June 4, 2026 16:08
@eskultety

Copy link
Copy Markdown
Member

@slimreaper35 So maybe squash like commit 1, 5, and 9 which would include "Moved npm package manager and it's unit tests under the /javascript folder; updates it's import paths to new location". And do the same with the rest which will bring it down to 4 commit?

For example, the first commit moves the whole package_managers/npm directory to package_managers/javascript/npm. But the tests still expect the previous package_managers/npm so they fail. It's related to the #1591 (comment).

Yes, the backend changes should be paired with the corresponding unit test changes in a single commit and we should be good (always verify with nox).

Mohamed Bouchtout added 4 commits June 9, 2026 11:59
…lder; updated import paths

Moved the NPM package manager and it's unit tests to the /javascript folder as part of the efforts to consolidate all javascript package managers. Updated all import paths accordingly.

Signed-off-by: Mohamed Bouchtout <mbouchto@redhat.com>
…older; updated import paths

Moved the yarn package manager and it's unit tests to the /javascript folder as part of the efforts to consolidate all javascript package managers. Updated all import paths accordingly.

Signed-off-by: Mohamed Bouchtout <mbouchto@redhat.com>
…/javascript folder; updated import paths

Moved the yarn_classic package manager and it's unit tests to the /javascript folder as part of the efforts to consolidate all javascript package managers. Updated all import paths accordingly.

Signed-off-by: Mohamed Bouchtout <mbouchto@redhat.com>
…ckage managers; updated import paths

Moved the metayarn.py file and it's unit test to the /javascript folder as part of the efforts to consolidate all javascript package managers. Updated all import paths accordingly.

Signed-off-by: Mohamed Bouchtout <mbouchto@redhat.com>
@mohamedbouchtout mohamedbouchtout force-pushed the feature/javascript-package-manager-relocation branch from 34a7c93 to 7046690 Compare June 9, 2026 16:08
@mohamedbouchtout

Copy link
Copy Markdown
Author

@slimreaper35 So maybe squash like commit 1, 5, and 9 which would include "Moved npm package manager and it's unit tests under the /javascript folder; updates it's import paths to new location". And do the same with the rest which will bring it down to 4 commit?

For example, the first commit moves the whole package_managers/npm directory to package_managers/javascript/npm. But the tests still expect the previous package_managers/npm so they fail. It's related to the #1591 (comment).

Yes, the backend changes should be paired with the corresponding unit test changes in a single commit and we should be good (always verify with nox).

I agree, so we don't have redundant commits. I've squashed the commits together and updated their message.

@eskultety

Copy link
Copy Markdown
Member

@mohamedbouchtout there are some merge conflicts here (not a coincidence since there's active work on the pnpm backend going on) that need resolving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants