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 handling of file: module URIs with non-ASCII characters #696

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

HT154
Copy link
Contributor

@HT154 HT154 commented Oct 16, 2024

Resolves #653

@HT154 HT154 force-pushed the nihongjouzu branch 3 times, most recently from df9ec5a to 2d75e6d Compare October 16, 2024 16:59
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

We should also fix this in org.pkl.core.module.FileResolver#listElements(java.net.URI), and org.pkl.core.module.FileResolver#hasElement(java.net.URI).

That would handle cases like:

read("file:///path/to/日本語.pkl")

pkl-core/src/main/java/org/pkl/core/module/ModuleKeys.java Outdated Show resolved Hide resolved
@HT154
Copy link
Contributor Author

HT154 commented Oct 17, 2024

I refactored this into IoUtils, adopted it in FileResolved and also StackFrameTransformer, and added some more test coverage.

I suspect most other usage of Path.of in the codebase is at risk of similar issues. Really quite an inconvenient way for this API to work!

@HT154 HT154 requested a review from bioball October 17, 2024 01:09
@HT154 HT154 requested a review from bioball October 17, 2024 19:04
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

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

LGTM pending CI checks.

You need to run ./gradlew spotlessApply to fix some formatting woes.

@HT154
Copy link
Contributor Author

HT154 commented Oct 18, 2024

Hmm I think this may be a legitimate difference in how things are handled on different OSs:

Changed content at line 6:
expecting:
  ["at 日本語_error#日本語 (file:///$snippetsDir/input/modules/%E6%97%A5%E6%9C%AC%E8%AA%9E_error.pkl)"]
but was:
  ["at 日本語_error#日本語 (file:///$snippetsDir/input/modules/日本語_error.pkl)"]

Is there a way to provide a different expectation or exclude this particular test on Windows? Or would it be best to just move this to CliEvaluatorTest where the URI diff can be substituted out of existence?

@bioball
Copy link
Contributor

bioball commented Oct 18, 2024

Let me look into that one for you; I have access to a Windows machine so it's much easier for me to figure this one out.

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.

Error occurs when processing files with Japanese names
3 participants