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.

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

internal: force use dunce canonicalize #347

Merged
merged 3 commits into from
Sep 26, 2024
Merged

Conversation

lijunchen
Copy link
Contributor

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Sep 26, 2024

From the provided git diff output, I've identified three notable changes that warrant attention:

  1. Addition of clippy.toml:

    • This new file introduces configuration for Clippy, Rust's linter. The specific addition of disallowed-methods with std::path::Path::canonicalize being disallowed suggests a preference for using dunce::canonicalize instead. This change is likely to enforce a more consistent or safer practice across the codebase, especially concerning path canonicalization which can be tricky on different platforms.
  2. Usage of dunce::canonicalize in test_cases/mod.rs and expect.rs:

    • The replacement of std::path::Path::canonicalize with dunce::canonicalize in multiple places (specifically in test_cases/mod.rs and expect.rs) directly aligns with the configuration in clippy.toml. This change likely aims to address potential issues with path canonicalization, especially on Windows where UNC paths can cause problems. Using dunce::canonicalize is a known workaround to handle these cases more gracefully.
  3. Modification in .justfile for Clippy command:

    • The addition of --locked to the cargo clippy command ensures that the exact dependencies specified in the Cargo.lock file are used. This is a good practice for reproducibility, especially in CI/CD environments where consistency is crucial. It prevents any surprises from newer versions of dependencies introducing changes that could affect the build or the linter results.

Suggestions:

  • Consistency Check: Ensure that all instances where std::path::Path::canonicalize is used are updated to use dunce::canonicalize. This will maintain consistency across the codebase and align with the linter configuration.

  • Review Impact: Given the introduction of a new linter configuration and changes to canonicalization methods, it might be beneficial to conduct a thorough review or additional testing, especially on Windows, to confirm that these changes do not introduce any unintended side effects.

  • Documentation Update: If dunce::canonicalize is preferred over std::path::Path::canonicalize, consider updating documentation or internal coding guidelines to reflect this preference. This will help new developers understand the reasoning behind the choice and encourage consistent practices.

These observations and suggestions should help maintain code quality and ensure that the changes are fully aligned with the project's standards and goals.

@lijunchen lijunchen force-pushed the force-use-dunce-canonicalize branch from b972ca1 to 7325c67 Compare September 26, 2024 06:53
@lijunchen lijunchen force-pushed the force-use-dunce-canonicalize branch from 7325c67 to 0bbc677 Compare September 26, 2024 07:12
@lijunchen lijunchen enabled auto-merge September 26, 2024 07:12
@lijunchen lijunchen merged commit ab6f59e into main Sep 26, 2024
9 checks passed
@lijunchen lijunchen deleted the force-use-dunce-canonicalize branch September 26, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant