Skip to content

Comments

fix #593: support metadata on nodes (5.x edition)#729

Merged
amitguptagwl merged 9 commits intoNaturalIntelligence:metadatafrom
srl295:line-number-593bis
Apr 3, 2025
Merged

fix #593: support metadata on nodes (5.x edition)#729
amitguptagwl merged 9 commits intoNaturalIntelligence:metadatafrom
srl295:line-number-593bis

Conversation

@srl295
Copy link
Contributor

@srl295 srl295 commented Feb 28, 2025 •
edited
Loading

  • add a new symbol, accessible via XMLParser.getMetaDataSymbol() which contains metadata on a node- such as the start index on a symbol.

Purpose / Goal

For our XML based parser, we want to show the line number of where an errant XML element was found.

This PR adds a symbol to objects in the output tree with the start index (in chars) of the element.

Fixes #593

Type

Please mention the type of PR

  • Bug Fix
  • Refactoring / Technology upgrade
  • New Feature

Benchmark

(TBD)

Bookmark this repository for further updates.

@srl295
fix NaturalIntelligence#593: support line numbers
7603798 
- add a new symbol, accessible via XMLParser.getStartIndexSymbol() which reflects the start index of a tag
- copy that start index in the compressed form as well

(cherry picked from commit d7601c3)
@srl295
fix NaturalIntelligence#593: support line numbers
72f2048 
- correct .d.ts
@srl295
Copy link
Contributor Author

srl295 commented Feb 28, 2025 •
edited
Loading

Tests fail as they don't expect the symbols to be present.
This may be another argument for a configuration option to request the symbols.

Tests passed on the 4.x version, perhaps a change in Jasmine now fails.

srl295 added a commit to keymanapp/keyman that referenced this pull request Feb 28, 2025
@srl295
feat(developer): API for resolving line numbers for messages
819d9bf 
- change to use a different fast-xml-parser PR, NaturalIntelligence/fast-xml-parser#729

Fixes: #10622
@srl295 srl295 mentioned this pull request Feb 28, 2025
Closed
3 tasks
@srl295
fix NaturalIntelligence#593: support line numbers
96325c4 
- add a preserveStartIndex option, off by default
- use an ordinary property if Symbol is not available
- update tests and docs
@srl295
Copy link
Contributor Author

srl295 commented Mar 4, 2025

Benchmark before (but see #730 ! )

Running Suite: XML Parser benchmark
fxp v3 : 52417.504792746455 requests/second
fxp : 30162.48878228146 requests/second
fxp - preserve order : 36884.27474631246 requests/second
xmlbuilder2 : 10806.911585238135 requests/second
xml2js  : 15039.625113158 requests/second

Benchmark after:

$ node benchmark/XmlParser.mjs 
Running Suite: XML Parser benchmark
fxp v3 : 49120.26149528948 requests/second
fxp : 29106.912630974275 requests/second
fxp - preserve order : 34027.03620847232 requests/second
xmlbuilder2 : 10509.771871726383 requests/second
xml2js  : 13547.130612654762 requests/second

@srl295 srl295 changed the base branch from master to metadata March 14, 2025 23:03
@srl295 srl295 marked this pull request as ready for review March 14, 2025 23:03
@srl295
Copy link
Contributor Author

srl295 commented Mar 14, 2025

@amitguptagwl ready to merge to 'metadata'

@amitguptagwl
Copy link
Member

amitguptagwl commented Mar 15, 2025 •
edited
Loading

Can you please change preserveStartIndex to captureMetaData and other changes accordingly. Can you please also include tests for other scenario as we discussed? Please also check if there is any impact on typings

@srl295
feat NaturalIntelligence#593: add metadata
194828b 
- add a captureMetaData which adds a metadata object
- update tests and typings
@srl295
Copy link
Contributor Author

srl295 commented Mar 17, 2025

captureMetaData

please take a look at the update now. (didn't update docs yet)

@amitguptagwl
Copy link
Member

amitguptagwl commented Mar 18, 2025

Thanks Steven, Can you please add tests for following?

  • isArray is true
  • stopNodes is set
  • unpairedTags is set
  • updateTag change tag name

@srl295
Copy link
Contributor Author

srl295 commented Mar 18, 2025 via email

@amitguptagwl
Copy link
Member

amitguptagwl commented Mar 19, 2025

I would be able to merge this PR after 3rd Apr.

@srl295
feat NaturalIntelligence#593: add metadata test
956138c 
- add test for captureMetadata && isArray && stopNodes && unpairedTags && updateTag
@srl295
Copy link
Contributor Author

srl295 commented Mar 19, 2025

@amitguptagwl take a look, see if the test is right. still need to update docs

@srl295
Copy link
Contributor Author

srl295 commented Mar 27, 2025

@amitguptagwl docs updated

@srl295 srl295 changed the title fix #593: support line numbers (5.x edition) fix #593: support metadata on nodes (5.x edition) Mar 28, 2025
@amitguptagwl
Copy link
Member

amitguptagwl commented Apr 2, 2025

Thanks for the changes. I'll have a look by tomorrow and confirm.

@amitguptagwl
Copy link
Member

amitguptagwl commented Apr 2, 2025

can you please update the fxp.d.cjs too? Please also merge latest changes of metadata branch.

@srl295
Copy link
Contributor Author

srl295 commented Apr 2, 2025

how do i update fxp.d.cjs?

@amitguptagwl
Copy link
Member

amitguptagwl commented Apr 2, 2025

https://github.com/NaturalIntelligence/fast-xml-parser/blob/master/lib/fxp.d.cts

@srl295
Copy link
Contributor Author

srl295 commented Apr 2, 2025 •
edited
Loading

https://github.com/NaturalIntelligence/fast-xml-parser/blob/master/lib/fxp.d.cts

the .cts - yes,

  • i will update the cts

but does https://github.com/NaturalIntelligence/fast-xml-parser/blob/master/lib/fxp.cjs also need to be updated? maybe with webpack npm run build or is that done as part of the release process?

@srl295
feat NaturalIntelligence#593: update .cts and .ts for metadata
916f979 
- remove duplicate X2jMetadata class
- update .cts file with metadata
@srl295
Copy link
Contributor Author

srl295 commented Apr 2, 2025

@amitguptagwl updated .cts

@srl295
Copy link
Contributor Author

srl295 commented Apr 2, 2025

@amitguptagwl merged up to date

@amitguptagwl amitguptagwl merged commit ab8d40a into NaturalIntelligence:metadata Apr 3, 2025
2 checks passed
@amitguptagwl
Copy link
Member

amitguptagwl commented Apr 3, 2025

changes are live. Thanks for your patience and apologies for delay.

@srl295 srl295 deleted the line-number-593bis branch May 8, 2025 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature Request: Allow including node metadata (startIndex) in output

2 participants

@srl295 @amitguptagwl