refactor: re-organize code #143

KSXGitHub · 2023-10-06T16:14:14Z

Summary

Move code that handles package managing to its own crate named pacquet_package_manager.
- Convert functions with multiple arguments into structs.
Make pacquet add reuses the algorithm from pacquet install.
- Achieved by having pacquet add creates an in-memory package.json and pass it to the Install subroutine.
Convert the error-prone tests in crates/cli into proper integration tests that invoke the pacquet binary.
- Tests become shorter and more readable.
- Solve race condition issues with cargo test.
All code with set_current_dir have been replaced by either integration tests or dependency injection.
- Solve race condition issues with cargo test.
thiserror has been completely removed. derive_more is used in its stead.
- thiserror cannot handle generic trait bound.
- derive_more is already used to generate custom string types in the lockfile.
- one less macro crate leads to slightly faster compile time.
pacquet_package_json::PackageJson has been renamed to pacquet_package_manifest::PackageManifest.
Other minor changes.

anonrig · 2023-10-06T16:17:29Z

Can you share the goal of this pull request before any reviews?

codecov · 2023-10-06T16:18:50Z

Codecov Report

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

Comparison is base (46604f5) 86.73% compared to head (47b3fa0) 86.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   86.73%   86.30%   -0.43%     
==========================================
  Files          39       53      +14     
  Lines        2540     2666     +126     
==========================================
+ Hits         2203     2301      +98     
- Misses        337      365      +28

Files	Coverage Δ
crates/cli/src/bin/main.rs	`100.00% <100.00%> (+100.00%)`	⬆️
crates/cli/src/lib.rs	`100.00% <100.00%> (+58.66%)`	⬆️
crates/fs/src/lib.rs	`100.00% <100.00%> (ø)`
crates/lockfile/src/comver.rs	`84.00% <ø> (ø)`
crates/lockfile/src/dependency_path.rs	`100.00% <ø> (ø)`
crates/lockfile/src/lib.rs	`0.00% <ø> (ø)`
crates/lockfile/src/load_lockfile.rs	`0.00% <ø> (ø)`
crates/lockfile/src/lockfile_version.rs	`100.00% <ø> (ø)`
crates/lockfile/src/pkg_name.rs	`96.87% <ø> (ø)`
crates/lockfile/src/pkg_name_suffix.rs	`96.87% <ø> (ø)`
... and 32 more

... and 1 file with indirect coverage changes

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

crates/package_manager/src/virtual_dir.rs

KSXGitHub · 2023-10-06T16:25:50Z

Can you share the goal of this pull request before any reviews?

The functions in pacquet_cli were named arbitrarily and placed to arbitrary modules, which make it difficult to navigate (I experienced it when I was doing the last frozen-lockfile PR). Furthermore, I see that pacquet_cli is loaded with responsibilities that aren't part of the CLI (such as fetching packages or modeling the node_modules folder structure). I want to move these code out to appropriate crates, with correct and descriptive names.

anonrig · 2023-10-16T19:03:37Z

It's impossible for me to review this pull request. It's 215 commits with 2000 lines. I recommend splitting the pull request into multiple pull requests.

anonrig · 2023-10-16T19:02:32Z

crates/cli/src/cli_args.rs

+                let command = if let Some(script) = manifest.script("start", true)? {
+                    script
+                } else {
+                    "node server.js"


This is wrong. We need to check for server.js. Here's the error code from pnpm

ERR_PNPM_NO_SCRIPT_OR_SERVER Missing script start or file server.js

Since I didn't write this code and it's not related to the goal of refactoring, I have created an issue ticket: #154

KSXGitHub · 2023-10-16T19:12:07Z

@anonrig

It's impossible for me to review this pull request. It's 215 commits with 2000 lines. I recommend splitting the pull request into multiple pull requests.

The PR is about restructuring the codebase. Not sure how splitting them is going to work.

You can read the summary at the top to get an idea of what it does. These bullet points closely depend on each other.

Co-authored-by: Zoltan Kochan <[email protected]>

zkochan · 2023-10-16T23:13:38Z

I think it is too late now to split this PR. In the future try to avoid such big PRs. If a PR's goal is to reorganize code then ideally it should only contain changes to file locations and/or parts of code moved between files. With no other changes.

zkochan · 2023-10-16T23:21:36Z

crates/package-manager/src/add.rs

+    pub manifest: &'a mut PackageManifest,
+    pub lockfile: Option<&'a Lockfile>,
+    pub list_dependency_groups: ListDependencyGroups, // must be a function because it is called multiple times
+    pub package_name: &'a str, // TODO: 1. support version range, 2. multiple arguments, 3. name this `packages`


it is not a package name because it may have a version or a tag specified too. Like express@1.

in pnpm will call it package_specifier or package_spec. I think npm calls it a package spec.

and I think Yarn calls it package_descriptor.

I would recommend using pnpm's naming conventions. If we decide to change some conventions, then we should change in both repositories.

Right now, it is package_name. The name would change once we add version range to pacquet add, but right now, it only functions as a package name.

zkochan · 2023-10-16T23:31:01Z

I will not post more comments as I don't know what code was just moved and what code is new. I am fine with merging it but in the future let's try to avoid mixing code changes and reorganization in a single PR. And let's try to make smaller PRs.

zkochan · 2023-10-17T13:55:14Z

@anonrig how do you feel about merging this?

anonrig · 2023-10-17T15:53:57Z

@anonrig how do you feel about merging this?

It includes refactoring and new changes that I'm not comfortable with merging. We should split refactoring and new features into separate pull requests.

anonrig

It's impossible to review this pull request due to the size and commit count. Please open separate pull request for refactoring and different pull request for each feature.

KSXGitHub · 2023-10-17T16:04:09Z

It's impossible to review this pull request due to the size and commit count. Please open separate pull request for refactoring and different pull request for each feature.

I object. The "features" are interdependent. Separating them will cause merge conflict.

anonrig · 2023-10-17T16:16:27Z

@KSXGitHub The pull request title and description does not mention this new features. How can a reviewer look through 200 commits and properly review them?

KSXGitHub · 2023-10-17T16:23:39Z

@anonrig #158 is the state of this PR right before @zkochan wrote this comment.

BTW, I have notified you guys in the Discord channel at the very moment this PR was drafted, and I've been pushing commits continuously since. I believe that GitHub should notifies you guys on each commit?

zkochan · 2023-10-18T14:23:26Z

@anonrig I think there is a misunderstanding here. This PR does not contain any changes to the CLI behaviour. All the changes that were done are in the PR description.

Maybe there was a way to do these changes in multiple PRs but now it is too late. There's no easy way to split them and I don't think we want @KSXGitHub to spend time on reapplying the changes from scratch.

I want to assure you that @KSXGitHub is a dedicated contributor that works on pnpm full time, so if we miss something in this PR, Khải will be there to work on the fixes.

KSXGitHub added 12 commits October 5, 2023 14:50

refactor: move the wrapper to its own crate

3415941

refactor: move symlink_pkg to package_manager

92fadde

chore(git): merge from main

16ab5e5

refactor: move auto_import

9604f46

refactor: rearrange dependencies

751c943

refactor: move create_virtdir_by_snapshot

05e7928

refactor: rename a mod

1cca0c5

refactor: move import package

3d81563

refactor: remove unused enum variant

4db8e06

refactor: use better names

a58343c

docs: link_file

92e9eea

refactor: move link_file to its own mod

207a87d

KSXGitHub requested review from zkochan and anonrig October 6, 2023 16:14

docs: correction

5155f95

anonrig reviewed Oct 6, 2023

View reviewed changes

crates/package_manager/src/virtual_dir.rs Outdated Show resolved Hide resolved

crates/package_manager/src/virtual_dir.rs Outdated Show resolved Hide resolved

crates/package_manager/src/virtual_dir.rs Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

KSXGitHub added 5 commits October 9, 2023 13:29

chore(git): merge from main

e1ea5fc

chore(cargo): move junction to dev-dependencies

f2e8f45

chore(cargo): make junction windows-only

8b00c83

refactor: rename

abfaddb

refactor: break a function into 2

61f00d3

KSXGitHub mentioned this pull request Oct 9, 2023

Rename the lockfile types #146

Open

KSXGitHub added 4 commits October 9, 2023 17:52

feat: propagate an error

a6cdcf5

refactor: convert a function into a struct

673c0d8

docs: CreateVirtualDirError

03d1f8b

style: mark procedure structs as must_use

f63a429

KSXGitHub added 6 commits October 16, 2023 20:16

docs: remove redundant comment

c25d2b3

refactor: rename package to package_name

400eb55

docs: future plan

bc9a454

docs: remove from fields

7ba09df

chore(git): merge from main

f6f5c40

fix(clippy): windows

78c4e1c

KSXGitHub force-pushed the big-refactor branch from 12624bf to 78c4e1c Compare October 16, 2023 18:35

anonrig reviewed Oct 16, 2023

View reviewed changes

KSXGitHub and others added 3 commits October 17, 2023 02:17

docs(cli): remove a "pacquet" mention

4e8463b

Co-authored-by: Zoltan Kochan <[email protected]>

docs: future plan

5a7ed67

chore(git): pull

c82a2a7

zkochan reviewed Oct 16, 2023

View reviewed changes

zkochan approved these changes Oct 16, 2023

View reviewed changes

chore(git): merge from main

8ea5089

anonrig requested changes Oct 17, 2023

View reviewed changes

chore(git): merge from main

47b3fa0

zkochan requested a review from anonrig October 26, 2023 09:26

GiladShoham approved these changes Oct 26, 2023

View reviewed changes

zkochan merged commit cbdff94 into main Oct 26, 2023
10 of 12 checks passed

zkochan deleted the big-refactor branch October 26, 2023 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: re-organize code #143

refactor: re-organize code #143

KSXGitHub commented Oct 6, 2023 •

edited

Loading

anonrig commented Oct 6, 2023

codecov bot commented Oct 6, 2023 •

edited

Loading

This comment was marked as outdated.

KSXGitHub commented Oct 6, 2023

anonrig commented Oct 16, 2023

anonrig Oct 16, 2023

KSXGitHub Oct 16, 2023

KSXGitHub commented Oct 16, 2023

zkochan commented Oct 16, 2023

zkochan Oct 16, 2023 •

edited

Loading

KSXGitHub Oct 17, 2023

zkochan commented Oct 16, 2023

zkochan commented Oct 17, 2023

anonrig commented Oct 17, 2023

anonrig left a comment

KSXGitHub commented Oct 17, 2023

anonrig commented Oct 17, 2023

KSXGitHub commented Oct 17, 2023 •

edited

Loading

zkochan commented Oct 18, 2023

refactor: re-organize code #143

refactor: re-organize code #143

Conversation

KSXGitHub commented Oct 6, 2023 • edited Loading

Summary

anonrig commented Oct 6, 2023

codecov bot commented Oct 6, 2023 • edited Loading

Codecov Report

This comment was marked as outdated.

KSXGitHub commented Oct 6, 2023

anonrig commented Oct 16, 2023

anonrig Oct 16, 2023

Choose a reason for hiding this comment

KSXGitHub Oct 16, 2023

Choose a reason for hiding this comment

KSXGitHub commented Oct 16, 2023

zkochan commented Oct 16, 2023

zkochan Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

KSXGitHub Oct 17, 2023

Choose a reason for hiding this comment

zkochan commented Oct 16, 2023

zkochan commented Oct 17, 2023

anonrig commented Oct 17, 2023

anonrig left a comment

Choose a reason for hiding this comment

KSXGitHub commented Oct 17, 2023

anonrig commented Oct 17, 2023

KSXGitHub commented Oct 17, 2023 • edited Loading

zkochan commented Oct 18, 2023

KSXGitHub commented Oct 6, 2023 •

edited

Loading

codecov bot commented Oct 6, 2023 •

edited

Loading

zkochan Oct 16, 2023 •

edited

Loading

KSXGitHub commented Oct 17, 2023 •

edited

Loading