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

convert accented characters to ascii before calculating the path size #25310

Closed

Conversation

amrani
Copy link

@amrani amrani commented Jun 7, 2016

Summary

Some characters, for i.e: ó, have a larger length than 1 due to it's byte size. When calculated the fixture path, the starting index is determined by the length of a path that may or may not contain a character like "ó". This may lead to incorrect fixture file paths.

To solve this, I used the transliterate method to convert the file_paths characters to ascii prior to calculating the length.

Other Information

Please review issue #25303 for more details.

@maclover7
Copy link
Contributor

@amrani Can you open up your PR against master? We can then backport to other branches from there :)

@sgrif
Copy link
Contributor

sgrif commented Jun 7, 2016

Can we just change this code to use .chars instead of messing with the encoding?

@amrani
Copy link
Author

amrani commented Jun 7, 2016

@sgrif
fixture_path before setting Dir:

ó.chars -> ["o", "́"]
ó.bytes -> [111, 204, 129]

fixture_path after setting Dir:

ó.chars -> ["ó"]
ó.bytes -> [195, 179]

@amrani
Copy link
Author

amrani commented Jun 7, 2016

@maclover7 I have not checked if this issue exists in the master branch. If it does not, is the issue closed without change? Or is the older branch (in this case, 4-2-stable) updated?

@maclover7
Copy link
Contributor

If the issue is not in master, then I believe this would still be accepted, since 4-2-stable receives bugfixes. In general, we prefer to send PRs to master, and then copy the changes over the other branches, rather than the other way around :)

@sgrif
Copy link
Contributor

sgrif commented Jun 7, 2016

@amrani You're missing my point. It doesn't matter whether ó is represented as one character or two. If we use .chars.length instead of .length, the result is the same. Your implementation would lose support for non-ascii paths which we would not accept.

@sgrif
Copy link
Contributor

sgrif commented Jun 7, 2016

And yes, this should be opened against master not 4-2-stable.

@sgrif sgrif closed this Jun 7, 2016
@amrani
Copy link
Author

amrani commented Jun 7, 2016

When the fixture_path is used in the following line, the resulting value seems to have converted or normalized the chars in fixture_path.
fixture_set_names = Dir["#{fixture_path}/{**,*}/*.{yml}"]
I tried chars.length and they are different sizes. Maybe I am still missing your point. Anyways, thanks for the quick response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants