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

Adjust test to error message change in #53778 #53813

Closed
wants to merge 1 commit into from
Closed

Conversation

Keno
Copy link
Member

@Keno Keno commented Mar 22, 2024

I considered changing the error message back, but I actually think non-escaped filenames make more sense here, so I just adjusted the test. We can do either though.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I think this breaks consistency with many other file methods (though I didn't check specifically). It seems somewhat rude to windows users to make it so they can't copy paste this text, even though the " make it look like a string

I considered changing the error message back, but I actually think
non-escaped filenames make more sense here, so I just adjusted
the test. We can do either though.
@Keno Keno force-pushed the kf/fix53778error branch from 9185aba to 299b0dd Compare March 22, 2024 09:39
@Keno
Copy link
Member Author

Keno commented Mar 22, 2024

I'll let you debate that with @JeffBezanson. I do find the double backslash somewhat ugly there. Maybe it should go into cmd ticks? Anyway, I just want CI to pass.

@IanButterworth
Copy link
Member

I believe this is still failing?

"opening file \"C:\\buildkite-agent\\builds\\win2k22-amdci6-4\\julialang\\julia-master\\julia-299b0dd524\\share\\julia\\stdlib\\v1.12\\Distributed\\test\\testfile\""
"opening file \"C:\\\\buildkite-agent\\\\builds\\\\win2k22-amdci6-4\\\\julialang\\\\julia-master\\\\julia-299b0dd524\\\\share\\\\julia\\\\stdlib\\\\v1.12\\\\Distributed\\\\test\\\\testfile\""

@giordano
Copy link
Contributor

giordano commented Mar 22, 2024

It'd also good to rebase on master to include #53809, which fixed #53811, and reduce CI noise.

@IanButterworth
Copy link
Member

IanButterworth commented Mar 22, 2024

In case this isn't resolved soon I've reopened and rebased the revert #53808 - Now merged

@JeffBezanson
Copy link
Member

Ok I think we can put the repr's back for now. But it would be better to call some more specific string escaping function if that's what's needed.

@JeffBezanson
Copy link
Member

Note the other error in that same function doesn't call repr, and I don't think we have a test for that error. I'm not sure how to trigger it.

@giordano giordano deleted the kf/fix53778error branch March 22, 2024 20:49
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.

5 participants