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

Fix File.dirname with unicode chars #8911

Merged
merged 2 commits into from
Mar 16, 2020
Merged

Fix File.dirname with unicode chars #8911

merged 2 commits into from
Mar 16, 2020

Conversation

asterite
Copy link
Member

Fixes #8910

There are a lot of + 1 in the code that might be worth reviewing. If someone wants they can try adding unicode tests to all of the File or Path methods to make sure they work well.

spec/std/file_spec.cr Outdated Show resolved Hide resolved
Co-Authored-By: Sijawusz Pur Rahnama <sija@sija.pl>
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Ooops.

@straight-shoota
Copy link
Member

I'll take a look at all implementations to make sure they work with unicode strings.

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:files labels Mar 16, 2020
@asterite asterite added this to the 0.34.0 milestone Mar 16, 2020
@asterite asterite merged commit 5cf185d into master Mar 16, 2020
@asterite asterite deleted the bug/file-dirname branch March 16, 2020 21:06
@straight-shoota
Copy link
Member

The rest of Path seems to be good. We could consider adding lots of specs with unicode paths to make sure there won't be any issues in the future.

@asterite
Copy link
Member Author

@straight-shoota Thank you for checking! If everything is good then I think we don't need to add those tests.

@Sija
Copy link
Contributor

Sija commented Mar 17, 2020

@straight-shoota Thank you for checking! If everything is good then I think we don't need to add those tests.

@asterite Well, it's good for now, we just need to subtly break the implementation to find out we need these tests anyway, so why wait for it if we can be prepared? 👑

@straight-shoota
Copy link
Member

Yes, having such test would be great. But it requires also quite some effort to implement and review. IMO that effort is currently better put to use elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File.dirname is broken for directories ending with a non-latin letter
4 participants