refactor path handling for cross-platform compatibility #59

mertcanaltin · 2023-08-07T20:56:46Z

This PR addresses the issue of inconsistent path handling across different operating systems in the get_all_folders function. The current implementation accepts both Unix and Windows path separators, leading to unpredictable behavior. The proposed solution aims to enhance cross-platform compatibility and improve code clarity

anonrig · 2023-08-07T21:19:27Z

Previous code was actually producing the same output for both windows and unix. Reverting those changes produce different path separators for files. On windows it is \ and on unix it is /.

anonrig · 2023-08-07T22:19:20Z

crates/cli/src/fs.rs

-                files.push(simple_path);
+            let simple_path = entry_path.strip_prefix(root).unwrap();
+            if !simple_path.as_os_str().is_empty() {
+                files.push(simple_path.to_string_lossy().to_string());


Does OsString produce the same string for both unix and windows? If yea, simple_path.as_os_string() is needed here.

yes, OsString produces the same string representation for both Unix and Windows systems. Thank you for pointing that out. I'll update the code to use simple_path.as_os_str() directly, which will ensure a more platform-independent approach. This change should contribute to better cross-platform compatibility

…nction

mertcanaltin · 2023-08-08T12:15:48Z

I made a small modification in the code by replacing .join("/") with .join(std::path::MAIN_SEPARATOR_STR) in the get_all_folders function. This change was suggested as a way to achieve better cross-platform consistency for file paths. By using std::path::MAIN_SEPARATOR_STR, we ensure that the path separators are correctly handled on both Unix and Windows systems, helping to avoid any potential issues or discrepancies in the future.

mertcanaltin · 2023-08-08T16:42:02Z

hello,
In the codebase, ı have come across an unused import warning related to the import std::{env, fs}

and one codecov alert

anonrig · 2023-08-08T16:46:57Z

Windows tests are still failing.

mertcanaltin · 2023-08-08T17:28:03Z

When I run the cargo test command on the main branch, I encounter errors in the snapshots. I'm wondering if I'm following the correct testing process. Here's the output:

➜  pacquet git:(main) cargo test
   Compiling pacquet_cli v0.0.1 (/Users/mert/Desktop/nodejs/pacquet/crates/cli)
    Finished test [unoptimized + debuginfo] target(s) in 3.61s
     Running unittests src/main.rs (target/debug/deps/pacquet_benchmark-d83e1adb6c3b3bb8)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/pacquet_cafs-ffac02af04ecec49)

running 2 tests
test tests::create_content_path_from_hex ... ok
test tests::should_write_and_clear ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/pacquet_cli-71cec2bc785727b0)

running 8 tests
test tests::init_command_should_throw_on_existing_file ... ok
test tests::should_get_store_path ... ok
test tests::init_command_should_create_package_json ... ok
test commands::add::tests::should_add_to_package_json ... ok
test commands::install::tests::should_install_dependencies ... FAILED
stored new snapshot /Users/mert/Desktop/nodejs/pacquet/crates/cli/src/commands/snapshots/pacquet_cli__commands__add__tests__should_symlink_correctly.snap.new
test commands::add::tests::should_symlink_correctly ... FAILED
test package::tests::should_find_package_version_from_registry ... ok
stored new snapshot /Users/mert/Desktop/nodejs/pacquet/crates/cli/src/commands/snapshots/pacquet_cli__commands__add__tests__should_install_all_dependencies.snap.new
test commands::add::tests::should_install_all_dependencies ... FAILED

failures:

---- commands::install::tests::should_install_dependencies stdout ----
thread 'commands::install::tests::should_install_dependencies' panicked at 'assertion failed: dir.path().join(\"node_modules/is-odd\").is_symlink()', crates/cli/src/commands/install.rs:117:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- commands::add::tests::should_symlink_correctly stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: crates/cli/src/commands/snapshots/pacquet_cli__commands__add__tests__should_symlink_correctly.snap
Snapshot: should_symlink_correctly
Source: crates/cli/src/commands/add.rs:195
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Expression: get_all_folders(&dir.path().to_path_buf())
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
    0       │-[
    1       │-    "node_modules",
    2       │-    "node_modules/.pacquet",
    3       │-    "node_modules/.pacquet/is-number@6.0.0",
    4       │-    "node_modules/.pacquet/is-number@6.0.0/node_modules",
    5       │-    "node_modules/.pacquet/is-number@6.0.0/node_modules/is-number",
    6       │-    "node_modules/.pacquet/is-odd@3.0.1",
    7       │-    "node_modules/.pacquet/is-odd@3.0.1/node_modules",
    8       │-    "node_modules/.pacquet/is-odd@3.0.1/node_modules/is-number",
    9       │-    "node_modules/.pacquet/is-odd@3.0.1/node_modules/is-odd",
   10       │-    "node_modules/is-odd",
   11       │-]
          0 │+[]
────────────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
thread 'commands::add::tests::should_symlink_correctly' panicked at 'snapshot assertion for 'should_symlink_correctly' failed in line 195', /Users/mert/.cargo/registry/src/index.crates.io-6f17d22bba15001f/insta-1.31.0/src/runtime.rs:563:9

---- commands::add::tests::should_install_all_dependencies stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: crates/cli/src/commands/snapshots/pacquet_cli__commands__add__tests__should_install_all_dependencies.snap
Snapshot: should_install_all_dependencies
Source: crates/cli/src/commands/add.rs:158
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Expression: get_all_folders(&dir.path().to_path_buf())
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
    0     0 │ [
    1     1 │     "node_modules",
    2     2 │     "node_modules/.pacquet",
          3 │+    "node_modules/.pacquet/fast-decode-uri-component@1.0.1",
          4 │+    "node_modules/.pacquet/fast-decode-uri-component@1.0.1/node_modules",
          5 │+    "node_modules/.pacquet/fast-decode-uri-component@1.0.1/node_modules/fast-decode-uri-component",
    3     6 │     "node_modules/.pacquet/is-buffer@1.1.6",
    4     7 │     "node_modules/.pacquet/is-buffer@1.1.6/node_modules",
    5     8 │     "node_modules/.pacquet/is-buffer@1.1.6/node_modules/is-buffer",
    6     9 │     "node_modules/.pacquet/is-buffer@1.1.6/node_modules/is-buffer/test",
┈┈┈┈┈┈┈┈┈┈┈┈┼┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
   11    14 │     "node_modules/.pacquet/is-number@3.0.0",
   12    15 │     "node_modules/.pacquet/is-number@3.0.0/node_modules",
   13    16 │     "node_modules/.pacquet/is-number@3.0.0/node_modules/is-number",
   14    17 │     "node_modules/.pacquet/is-number@3.0.0/node_modules/kind-of",
         18 │+    "node_modules/.pacquet/is-number@6.0.0",
         19 │+    "node_modules/.pacquet/is-number@6.0.0/node_modules",
         20 │+    "node_modules/.pacquet/is-number@6.0.0/node_modules/is-number",
   15    21 │     "node_modules/.pacquet/is-odd@0.1.2",
   16    22 │     "node_modules/.pacquet/is-odd@0.1.2/node_modules",
   17    23 │     "node_modules/.pacquet/is-odd@0.1.2/node_modules/is-number",
   18    24 │     "node_modules/.pacquet/is-odd@0.1.2/node_modules/is-odd",
         25 │+    "node_modules/.pacquet/is-odd@3.0.1",
         26 │+    "node_modules/.pacquet/is-odd@3.0.1/node_modules",
         27 │+    "node_modules/.pacquet/is-odd@3.0.1/node_modules/is-number",
         28 │+    "node_modules/.pacquet/is-odd@3.0.1/node_modules/is-odd",
   19    29 │     "node_modules/.pacquet/kind-of@3.2.2",
   20    30 │     "node_modules/.pacquet/kind-of@3.2.2/node_modules",
   21    31 │     "node_modules/.pacquet/kind-of@3.2.2/node_modules/is-buffer",
   22    32 │     "node_modules/.pacquet/kind-of@3.2.2/node_modules/kind-of",
         33 │+    "node_modules/fast-decode-uri-component",
   23    34 │     "node_modules/is-even",
         35 │+    "node_modules/is-odd",
   24    36 │ ]
────────────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
thread 'commands::add::tests::should_install_all_dependencies' panicked at 'snapshot assertion for 'should_install_all_dependencies' failed in line 158', /Users/mert/.cargo/registry/src/index.crates.io-6f17d22bba15001f/insta-1.31.0/src/runtime.rs:563:9


failures:
    commands::add::tests::should_install_all_dependencies
    commands::add::tests::should_symlink_correctly
    commands::install::tests::should_install_dependencies

test result: FAILED. 5 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.92s

error: test failed, to rerun pass `-p pacquet_cli --lib`

This reverts commit 1cd3f84.

codecov-commenter · 2023-08-09T09:39:46Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (2980669) 85.40% compared to head (5d08ac9) 85.41%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
+ Coverage   85.40%   85.41%   +0.01%     
==========================================
  Files          24       24              
  Lines        1247     1248       +1     
==========================================
+ Hits         1065     1066       +1     
  Misses        182      182

Files Changed	Coverage Δ
crates/cli/src/fs.rs	`100.00% <100.00%> (ø)`

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

mertcanaltin · 2023-08-10T19:33:16Z

I think I passed the tests

anonrig

The existing solution behaves the same way, and does not require the creation of OsString

mertcanaltin · 2023-08-10T19:49:53Z

I'm glad I learned this. It was a nice little educational experience for me, and it. Thank you very much ❤️

refactor path handling for cross-platform compatibility

dfefb03

mertcanaltin mentioned this pull request Aug 7, 2023

We need this mutation to ensure that both Unix and Windows paths resolves the same #60

Closed

anonrig reviewed Aug 7, 2023

View reviewed changes

mertcanaltin added 13 commits August 8, 2023 10:31

fix: Use platform-independent path conversion in get_all_folders

8b926c5

refactor: Use direct path conversion for simple_path

2dd66f2

refactor: Use direct path conversion for simple_path

594ad98

use platform-independent path conversion in get_all_folders

4480756

cross-platform path handling and folder retrieval

3dec60f

added seperator

00a839f

fix path conversion for join() method

619f5e2

revert changes

080fb7b

added simple_path.as_os_string()

6382431

fix null pointer dereference in process handling

aba2ecd

fix path joining with correct separator

bbcf3a3

fix path joining with correct separator constant

240b513

Apply cross-platform consistency for file paths in get_all_folders fu…

8ba816b

…nction

mertcanaltin added 2 commits August 8, 2023 15:28

fix path conversion in get_all_folders function

53ef468

lint

f1267f8

mertcanaltin requested a review from anonrig August 8, 2023 16:42

mertcanaltin added 6 commits August 8, 2023 21:24

delete OsString for simple_path

de6284e

revert changes for join seperator

86f62c9

added MAIN_SEPARATOR_STR for join

37d1089

added OsString for simple_path

60e750d

fix: Fix os_string usage in get_all_folders function

cae3d44

update snapshots

1cd3f84

mertcanaltin added 5 commits August 8, 2023 21:58

Revert "update snapshots"

3240f42

This reverts commit 1cd3f84.

fixed: path handling for cross-platform compatibility

d253398

to_string_lossy delete

5c8082f

import osString

29ef018

re import osString

510fc15

mertcanaltin added 5 commits August 10, 2023 22:11

updated code for cross-platform compatibility

fafdd3a

Merge branch 'main' into path-separator-improvements

fc2369d

imports repair lint

79f0f2d

Refactored path handling for cross-platform compatibility

0ddf5fa

revert changes

5d08ac9

anonrig requested changes Aug 10, 2023

View reviewed changes

anonrig closed this Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor path handling for cross-platform compatibility #59

refactor path handling for cross-platform compatibility #59

mertcanaltin commented Aug 7, 2023

anonrig commented Aug 7, 2023

anonrig Aug 7, 2023

mertcanaltin Aug 7, 2023

mertcanaltin commented Aug 8, 2023

mertcanaltin commented Aug 8, 2023 •

edited

anonrig commented Aug 8, 2023

mertcanaltin commented Aug 8, 2023 •

edited

codecov-commenter commented Aug 9, 2023 •

edited

mertcanaltin commented Aug 10, 2023

anonrig left a comment

mertcanaltin commented Aug 10, 2023

refactor path handling for cross-platform compatibility #59

refactor path handling for cross-platform compatibility #59

Conversation

mertcanaltin commented Aug 7, 2023

anonrig commented Aug 7, 2023

anonrig Aug 7, 2023

Choose a reason for hiding this comment

mertcanaltin Aug 7, 2023

Choose a reason for hiding this comment

mertcanaltin commented Aug 8, 2023

mertcanaltin commented Aug 8, 2023 • edited

anonrig commented Aug 8, 2023

mertcanaltin commented Aug 8, 2023 • edited

codecov-commenter commented Aug 9, 2023 • edited

Codecov Report

mertcanaltin commented Aug 10, 2023

anonrig left a comment

Choose a reason for hiding this comment

mertcanaltin commented Aug 10, 2023

mertcanaltin commented Aug 8, 2023 •

edited

mertcanaltin commented Aug 8, 2023 •

edited

codecov-commenter commented Aug 9, 2023 •

edited