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 EntityResolver and DocType event #583

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

pigeonhands
Copy link
Contributor

@pigeonhands pigeonhands commented Mar 29, 2023

fixes #581

  • Added PayloadEvent::DocType
  • Added an EntityResolver so that unknown entities can be resolved.
  • Changed unsescape_ functions to use FnMut instead of Fn for resolve_entity parameters.

@dralley
Copy link
Collaborator

dralley commented Mar 29, 2023

Please add an entry to the changelog and fix the typo in the message of the last commit

@pigeonhands pigeonhands force-pushed the feature/entity-resolver branch from 236086f to 060b565 Compare March 29, 2023 22:38
@pigeonhands
Copy link
Contributor Author

@dralley Done

Copy link
Collaborator

@Mingun Mingun 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 work! I think PR will benefit if you make such changes: split it into three commits (rebase your branch and force push it):

  • the first one that synchronizes documentation between variants of Event and PayloadEvent
  • the second one which relaxes requirements to resolve_entity parameters from Fn to FnMut. That commit should relax requirements in all places and note this in the changelog
  • the third one which adds an EntityResolver trait etc. I was too quick in my comment regarding the resolve method. Because resolve_entity are now would be FnMut, the trait could be defined as
    pub trait EntityResolver {
        fn capture(&mut self, doctype: BytesText);
        fn resolve(&mut self, entity: &str) -> Option<&str>;
    }
    Add tests for the new feature in this commit

src/de/mod.rs Outdated Show resolved Hide resolved
) -> Result<Cow<'input, str>, EscapeError>
where
// the lifetime of the output comes from a capture or is `'static`
F: Fn(&str) -> Option<&'entity str>,
F: FnMut(&str) -> Option<&'entity str>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After consulting with the standard library I think, it's a good idea, but that change should be done in it's own commit and have it's own changelog entry.

Copy link
Collaborator

@dralley dralley Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not immediately clear why it's necessary, as the docs say:

Use FnMut as a bound when you want to accept a parameter of function-like type and need to call it repeatedly, while allowing it to mutate state. If you don’t want the parameter to mutate state, use Fn as a bound; if you don’t need to call it repeatedly, use FnOnce.

What state do you expect the entity resolution process to mutate? I would think it would be read-only... Or is it more a matter of just providing the flexibility anyway if there are no downsides to using FnMut in our context?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is just to provide flexibility and be consistent with similar std API, such as Iterator::map.

src/resolver.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
@pigeonhands pigeonhands force-pushed the feature/entity-resolver branch from adaedb0 to cd42ced Compare March 30, 2023 07:40
@pigeonhands pigeonhands changed the title Add entity resolver and DocType event Add EntityResolver and DocType event Mar 30, 2023
Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost good, just need to polish some things.

Actually, the first commit did not what I want. I meant that you to change the doc strings of all matching Event and PayloadEvent variants to be the same by keeping the best description. And then in the third commit add a new variant to PayloadEvent (so it would be just a copy of the corresponding variant from Event)

src/de/map.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/de/mod.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
tests/serde-de.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
src/de/var.rs Outdated Show resolved Hide resolved
src/de/map.rs Outdated Show resolved Hide resolved
tests/serde-de.rs Outdated Show resolved Hide resolved
tests/serde-de.rs Outdated Show resolved Hide resolved
@pigeonhands pigeonhands force-pushed the feature/entity-resolver branch 3 times, most recently from d9f2446 to d99f613 Compare March 31, 2023 00:06
@pigeonhands
Copy link
Contributor Author

pigeonhands commented Mar 31, 2023

@Mingun

This is almost good, just need to polish some things.

Actually, the first commit did not what I want. I meant that you to change the doc strings of all matching Event and PayloadEvent variants to be the same by keeping the best description. And then in the third commit add a new variant to PayloadEvent (so it would be just a copy of the corresponding variant from Event)

I'm not sure what you mean by this. Should all the enum variants be the same between PayloadEvent and Event or just the docs?

@Mingun
Copy link
Collaborator

Mingun commented Mar 31, 2023

Just the docs, because PayloadEvent is a subset of possible Events, and docs should be the same on same variants.

@Mingun Mingun force-pushed the feature/entity-resolver branch from d99f613 to 9b5e0e9 Compare April 8, 2023 16:54
Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR, with the following changes:

  • Allow EntityResolver::capture method to fail
  • Change the example on EntityResolver more close to possible real world implementation
  • Make resolver module private and instead re-export trait and type in de module

Unfortunately, making EntityResolver::resolve taking &mut self leads to strange error, that the returned reference could not outlive the captured object in the closure. This is strange, because returning the same reference while resolver is read-only is fine. So if you wish to make lazy parsing in resolve, you should use Cell or similar.

@dralley, could you look at this once again? It seems good for me now and I plan to make another patch release after merging that.

@Mingun Mingun requested a review from dralley April 8, 2023 17:04
@codecov-commenter
Copy link

Codecov Report

Merging #583 (9b5e0e9) into master (9c60819) will increase coverage by 0.17%.
The diff coverage is 83.33%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
+ Coverage   64.19%   64.36%   +0.17%     
==========================================
  Files          34       35       +1     
  Lines       16631    16676      +45     
==========================================
+ Hits        10676    10734      +58     
+ Misses       5955     5942      -13     
Flag Coverage Δ
unittests 64.36% <83.33%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/events/attributes.rs 93.56% <0.00%> (ø)
src/de/resolver.rs 14.28% <14.28%> (ø)
src/de/mod.rs 70.55% <95.55%> (+0.84%) ⬆️
src/de/map.rs 83.93% <100.00%> (ø)
src/de/var.rs 90.62% <100.00%> (ø)
src/escapei.rs 13.24% <100.00%> (ø)
src/events/mod.rs 67.50% <100.00%> (+1.66%) ⬆️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

Optional unescaping with serde
4 participants