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.

Sign up for GitHub

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

Jump to bottom

Fix home_dir() and current_dir() regression #3938

Merged
merged 4 commits into from
Jul 12, 2024
Merged

Fix home_dir() and current_dir() regression #3938

merged 4 commits into from
Jul 12, 2024

Conversation

djc
Copy link
Contributor

@djc djc commented Jul 11, 2024 •
edited
Loading

In #3764 (specifically 204c8a9), I moved a bunch of code around and in the process mixed up the OS and Test cases. Fix my mistake.

Thanks @ChrisDenton for paying attention and bisecting!

As discussed in #3936.

@djc djc requested a review from rami3l July 11, 2024 08:22
ChrisDenton
ChrisDenton approved these changes Jul 11, 2024
View reviewed changes
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

Thanks djc!

rami3l
rami3l approved these changes Jul 11, 2024
View reviewed changes
Copy link
Member

@rami3l rami3l left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot! However the implementation of var_os() below is still a bit verbose... Can't we just call Process::var_os() directly without the match?

@djc
Copy link
Contributor Author

djc commented Jul 11, 2024

Thanks a lot! However the implementation of var_os() below is still a bit verbose... Can't we just call Process::var_os() directly without the match?

Good call! Also tacked on two more commits with some quick renaming.

@rami3l rami3l mentioned this pull request Jul 11, 2024
Closed
2 tasks
rami3l
rami3l approved these changes Jul 11, 2024
View reviewed changes
Copy link
Member

@rami3l rami3l left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM modulo some rustc/clippy errors. Thanks again!

(fcb31a1 has passed my manual tests via UTM WoA on my Mac.)

@rami3l rami3l force-pushed the fix-dir-regression branch from 5f20b19 to 452d9f9 Compare July 12, 2024 01:02
@rami3l rami3l force-pushed the fix-dir-regression branch from 452d9f9 to 52157df Compare July 12, 2024 01:03
@rami3l rami3l enabled auto-merge July 12, 2024 01:23
@rami3l rami3l added this pull request to the merge queue Jul 12, 2024
Merged via the queue into master with commit 98080c3 Jul 12, 2024
27 checks passed
@rami3l rami3l deleted the fix-dir-regression branch July 12, 2024 01:48
@djc
Copy link
Contributor Author

djc commented Jul 12, 2024

Thanks for cleaning this up for me, wasn't able to follow up yesterday!

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

@rami3l rami3l rami3l approved these changes

@ChrisDenton ChrisDenton ChrisDenton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@djc @ChrisDenton @rami3l