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

feat(developer): support line numbers for LDML compiler errors #10622

Open
mcdurdin opened this issue Feb 5, 2024 · 7 comments
Open

feat(developer): support line numbers for LDML compiler errors #10622

mcdurdin opened this issue Feb 5, 2024 · 7 comments
Assignees
Labels
blocked Blocked by another issue or PR developer/ epic-ldml feat
Milestone

Comments

@mcdurdin
Copy link
Member

mcdurdin commented Feb 5, 2024

Our messages (errors, hints, and warnings) don't have line numbers at present, which is going to be quite painful for users!

Originally posted by @mcdurdin in #10614 (comment)

@srl295
Copy link
Member

srl295 commented Feb 5, 2024

Sorry, I was in file mode and didn't see this:

per #10614 quoth @mcdurdin

Our messages (errors, hints, and warnings) don't have line numbers at present, which is going to be quite painful for users!

right, I'm not sure how we can get that out of the xml parser though.

What I did for CLDR is have the parser record a xpath-to-line number mapping, with a custom SAX parser. If we had something similar here (.../key[@id="hamza"] or .../transformGroup[@3]/transform[@2] for 'the second transform in the third group') could be collected in the CompilerEvent, and then (probably) re-parse the XML (expanding imports!) looking for something with a match.

My point is that where the error actually occurs there isn't any context of the actual XML, but it might have enough section-relative data to be able to construct an xpath.

The slightly-related topic here is that we could also deduplicate errors at that point as well, particularly errors on the same part of the XML. "This key has these errors.."

What do you think?

@mcdurdin
Copy link
Member Author

mcdurdin commented Feb 5, 2024

right, I'm not sure how we can get that out of the xml parser though.

Relates to #8616 (comment) and choices we make in the future. Sounds like maintaining line/char position could be a required feature for our xml parser of choice?

My point is that where the error actually occurs there isn't any context of the actual XML, but it might have enough section-relative data to be able to construct an xpath.

So tracking context throughout the compiler would also be helpful -- whether we do that by xpath or by line/char, either will work.

This is also going to be useful for interactive debugging, assuming we go there (which would be a new section in the KMXPlus format).

@srl295
Copy link
Member

srl295 commented Feb 6, 2024

Perhaps we could use JavaScript symbols for the Line information, similar to what I'm already doing for import information

@srl295
Copy link
Member

srl295 commented Feb 13, 2024

CompilerEvent already has:

export interface CompilerEvent {
  filename?: string;
  line?: number;
  code: number;
  message: string;
  /**
   * an internal error occurred that should be captured with a stack trace
   * e.g. to the Keyman sentry instance by kmc
   */
  exceptionVar?: any;
};

filename/line aren't currently used, at least by ldml. I kind of wonder if they should be an array, that way there could be multiple lines/files with the same error? But, that could be coalesced later.

Also an xpath or something could go into this struct.

@mcdurdin
Copy link
Member Author

filename/line aren't currently used, at least by ldml

They are used by the other compilers though!

I like the simplicity of this structure as it is. Adding an xpath could be useful but xpath needs to be translated back to file+line for editor use anyway.

@mcdurdin
Copy link
Member Author

mcdurdin commented Feb 15, 2024

#10733 is a dup. Example of unhelpful message:

ldml_test.xml: Error: 0007 The source file has an invalid structure: Unable to load XML file

See comments on that issue for additional work here.

@mcdurdin mcdurdin modified the milestones: 18.0, A18S4 Apr 29, 2024
@darcywong00 darcywong00 modified the milestones: A18S4, A18S5 Jun 21, 2024
@darcywong00 darcywong00 modified the milestones: A18S5, A18S6 Jul 8, 2024
@darcywong00 darcywong00 modified the milestones: A18S6, A18S7 Jul 19, 2024
@darcywong00 darcywong00 modified the milestones: A18S7, A18S8 Aug 2, 2024
@darcywong00 darcywong00 added the blocked Blocked by another issue or PR label Aug 6, 2024
@darcywong00 darcywong00 modified the milestones: A18S8, A18S19 Aug 6, 2024
@darcywong00
Copy link
Contributor

Moving to A18S19, blocked by #5016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by another issue or PR developer/ epic-ldml feat
Projects
Status: No status
Development

No branches or pull requests

3 participants