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 nested fragments #12

Merged
merged 6 commits into from
Jun 5, 2024
Merged

Add nested fragments #12

merged 6 commits into from
Jun 5, 2024

Conversation

elidhu
Copy link
Contributor

@elidhu elidhu commented Apr 12, 2024

I have added nested fragment parsing. I have not had to touch any tests except:

  • Removing the "double open" test as it is no longer an error.
  • Adding a "halt while open" test that I noticed was missing.
  • Adding a "nested fragment" test.

The original behaviour should be identical as far as the tests go.

The new behaviour can be seen in the example I added. The most notable part is the handling of nested fragment tags when outputting a fragment that has children. Nested tags are excluded as if they weren't there.

Let me know your thoughts.

Thanks!

@elidhu elidhu force-pushed the master branch 3 times, most recently from b53e054 to 96acc13 Compare April 12, 2024 14:10
Copy link
Owner

@nickpascucci nickpascucci 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 your PR! This looks good, nice work.

Before I approve, can I ask you to update the README with:

  • A mention that fragments can be nested
  • Removing the "future work" item
  • Adding yourself to the "Contributors" section

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@elidhu
Copy link
Contributor Author

elidhu commented Apr 14, 2024

Addressed all of the above, and updated the README.

Thanks for taking the time to review!

@elidhu
Copy link
Contributor Author

elidhu commented Apr 14, 2024

I did have the thought while I was making the changes that this adds support for "nested" fragments, not necessarily "overlapping". This would required some sort of ID on the closing tag (I guess).

I'm not sure if that was an important distinction

@elidhu elidhu requested a review from nickpascucci April 14, 2024 23:30
@nickpascucci
Copy link
Owner

Yes, good point — I don't think it's a hugely important distinction, and as you said, once the fragment structure is no longer tree-like it becomes necessary to annotate the closing tags. This seems like a good next step, adding a nice feature without complecting the system too much.

elidhu added 6 commits April 15, 2024 21:23
Signed-off-by: Kevin Glasson <[email protected]>
Signed-off-by: Kevin Glasson <[email protected]>
- Rename error variant DoubleOpen -> UnclosedFragment.
- Add clarifying comments to nested fragment test.

Signed-off-by: Kevin Glasson <[email protected]>
Signed-off-by: Kevin Glasson <[email protected]>
@elidhu
Copy link
Contributor Author

elidhu commented Apr 15, 2024

RIght, had to fix all my signatures. It's good-to-go now!

@elidhu
Copy link
Contributor Author

elidhu commented May 15, 2024

Would you be able to get this merged and cut a release? 🙏

@nickpascucci nickpascucci merged commit 5da2a49 into nickpascucci:master Jun 5, 2024
1 check passed
@nickpascucci
Copy link
Owner

@elidhu sorry for the wait — I've just published v0.3.0 which includes your change 🎉

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