Skip to content

Conversation

MilesCranmer
Copy link
Member

@MilesCranmer MilesCranmer commented Aug 24, 2025

Cherry picked from #1230

Old function is deprecated

@IanButterworth IanButterworth requested a review from Copilot August 26, 2025 13:09
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a deprecation warning by updating the method used to get the path from a temporary directory object. The old into_path() method has been deprecated and replaced with path().

  • Replace deprecated into_path() calls with path() method
  • Update two instances in the install_from_url function

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@IanButterworth
Copy link
Member

I asked Claude about copilots review.. (😅) and it said this:


Based on my analysis, the Copilot PR review is INCORRECT and misleading. Here's why:

The Real Issue

The PR is trying to address a legitimate deprecation warning. I confirmed that TempDir::into_path() is indeed deprecated in tempfile 3.21+ (the latest version), with the deprecation message:

use of deprecated method `tempfile::TempDir::into_path`: use TempDir::keep()

Why the Review is Wrong

  1. Incorrect Solution: The AI reviewer suggests reverting to into_path(), but this is the deprecated method that caused the warning in the first place.

  2. Misunderstands the Use Cases:

    • Case 1 (remove_dir_all): This is in an error handling branch where we want to clean up manually. Using path() is actually safer here because the TempDir will still handle cleanup automatically if our manual cleanup fails.
    • Case 2 (rename): While the reviewer has a point about ownership semantics, using path() is still safe because rename is an atomic operation and the directory will be moved before TempDir tries to clean it up.
  3. Missing the Real Fix: The correct solution should be replacing into_path() with keep(), not reverting to the deprecated method.

Correct Solution

The PR author should use keep() instead of path() for cases where they want to prevent automatic cleanup:

// For the rename case:
std::fs::rename(temp_dir.keep(), &target_path)?;

// For the error case, path() is actually fine:
std::fs::remove_dir_all(temp_dir.path())?;

Conclusion

The Copilot review demonstrates a common issue with AI code reviewers - it identified a potential semantic difference but failed to consider:

  1. The actual deprecation warning that motivated the change
  2. The specific use cases and their safety implications
  3. The correct modern API to use (keep())

The review should be rejected as it would reintroduce deprecated code rather than properly fixing the deprecation warning.

@MilesCranmer
Copy link
Member Author

Lol yeah both are wrong

@MilesCranmer
Copy link
Member Author

@IanButterworth you should set up https://github.com/upstash/context7 with your claude, so it can more easily query up-to-date docs

@MilesCranmer
Copy link
Member Author

Folded into #1232

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.

2 participants