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

Add a stringifyGPX method to serialize a ParsedGPX object as XML. #10

Merged
merged 3 commits into from
Sep 1, 2024

Conversation

bdferris-v2
Copy link
Contributor

As discussed in issue #9.

@We-Gold We-Gold requested review from We-Gold and removed request for We-Gold August 29, 2024 21:10
@We-Gold
Copy link
Owner

We-Gold commented Aug 29, 2024

Overall, looks good, I'll make sure to run the tests myself later to make sure everything works for me too. The next improvement to the main functionality of the library (outside of this PR) will probably be rewriting the parsing to be more modular, and this code would provide a solid basis for that functionality.

One request: the parsing feature of the library supports custom parsers, other than the browser-provided parser for use in frameworks like React Native. It would be great if this stringification feature also supported that. A glance at the xmldom-qsa (the library I've been testing with) README reveals that a custom XMLSerializer is available through the library.

Edit: Confirmed that the tests pass locally. I realized it would also be great if the PR could include some documentation for this feature in the README as well.

@bdferris-v2
Copy link
Contributor Author

Cool, I update README.md and index.ts with details on stringifyGPX. I also added support for a custom XMLSerializer. Let me know if that's what you had in mind.

@We-Gold
Copy link
Owner

We-Gold commented Aug 31, 2024

Yep, that's what I was aiming for.

However, I believe the line in the README where you show how to use the custom serializer has a bug.

Rather than

const xml = stringifyGPX(parsedFile, customXmlSerializer = new XMLSerializer());

it should be

const xml = stringifyGPX(parsedFile, new XMLSerializer());

Otherwise, it will throw an error saying "customXmlSerializer is not defined" unless you previously defined it.

Also, could you add another test like this to test the case with a custom parser?

test("converts ParsedGPX to string", () => {
    const [gpx, error] = parseGPX(testGPXFile);
    const xml = stringifyGPX(gpx, new XMLSerializer()); // <-- This is imported from xmldom-qsa
    expect(prettyPrintXml(xml)).toEqual(prettyPrintXml(EXPECTED_XML));
})

Thanks!

@bdferris-v2
Copy link
Contributor Author

Done and done.

@We-Gold We-Gold merged commit dcdaa57 into We-Gold:main Sep 1, 2024
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.

2 participants