-
Notifications
You must be signed in to change notification settings - Fork 155
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
Correctly strip Windows paths from error messages #304
Conversation
val expected = getExpected(fn) | ||
val (_, problems) = JavaKSYParser.localFileToSpecs(fn, DEFAULT_CONFIG) | ||
val expected = getExpected(f) | ||
val (_, problems) = JavaKSYParser.localFileToSpecs(f.getPath(), DEFAULT_CONFIG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I think it will be meaningful to pass File
object to localFileToSpecs
, but that method is called also in test generator in https://github.com/kaitai-io/kaitai_struct_tests repository, so I doesn't touch it yet.
I don't think we should proceed this way - this PR only works around the effect of the bug instead of addressing its cause. Read kaitai-io/kaitai_struct_tests#124 (comment) again.
Nope, that would be a regression because it has the unpleasant consequence described in issue kaitai-io/kaitai_struct#507 (comment) that you are clearly not aware of. |
What in this PR contradicts the information in kaitai-io/kaitai_struct_tests#124 (comment)? If I understand correctly, it is fine to have platform-specific delimiters in paths in error messages. This PR just create the same platform-specific path and strip it from the output. What's wrong with that approach? What bug you mean? Do you have a link to it? I saw kaitai-io/kaitai_struct#507 earlier and having platform-specific delimiters was found to be expected behavior, no? Even if you think that some bug is exist, it is not the reason to make life harder. If you find the time to fix that bug, you also will able to fix that place too, because you will see new failing tests. |
I'm afraid, that the current solution is the best way to dealing with paths in the most agnostic way. The platform-specific path always could be present in the error message in case of non-existing imports:
The kaitai_struct_compiler/jvm/src/main/scala/io/kaitai/struct/formats/JavaKSYParser.scala Lines 39 to 46 in 2191070
Current implementation contains OS-dependent path. It is possible to hack the error message, replace that path and rethrow the exception, but it seems too hacky. I think, that instead of try to keep original paths in the ksc output, ksc can return a dictionary with mapping of provided paths to the actually used strings in messages. That way utilities which want to index JSON with paths should get a normalized path from the dictionary and use it. |
This PR correctly strips
..\tests\formats_err\
string from the error messages on Windows, while still strips../tests/formats_err/
on *nix.