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

basic custom quote support #544

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TheBotlyNoob
Copy link

No description provided.

@codecov-commenter
Copy link

Codecov Report

Merging #544 (fdb46f5) into master (add7406) will increase coverage by 0.30%.
The diff coverage is 86.49%.

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
+ Coverage   61.28%   61.58%   +0.30%     
==========================================
  Files          32       32              
  Lines       15639    15837     +198     
==========================================
+ Hits         9584     9754     +170     
- Misses       6055     6083      +28     
Flag Coverage Δ
unittests 61.58% <86.49%> (+0.30%) ⬆️

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

Impacted Files Coverage Δ
src/reader/ns_reader.rs 63.71% <ø> (ø)
src/events/mod.rs 70.58% <25.00%> (-1.06%) ⬇️
src/reader/mod.rs 94.57% <80.95%> (-0.49%) ⬇️
src/events/attributes.rs 92.90% <88.54%> (-0.59%) ⬇️
src/de/map.rs 72.36% <100.00%> (-0.44%) ⬇️
src/name.rs 87.85% <100.00%> (+0.02%) ⬆️
src/reader/buffered_reader.rs 85.44% <100.00%> (+0.06%) ⬆️
src/reader/slice_reader.rs 100.00% <100.00%> (ø)
... and 3 more

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

@Mingun
Copy link
Collaborator

Mingun commented Jan 21, 2023

Thanks for PR! Could you please describe your use-case, why you need quotes other than " or '?

You should run cargo fmt and at the same time you could squash all commints into one and --force-with-lease push it.

Also note, that each new feature requires a test cases for it, but I think, we could wait with that until you describes rationale for this change. Especially confusing for me why you need different open and close delimiters?

basic custom quote support

remove extra file

more consistent styling

allow actually using the feature

fix logic issue

fmt
@TheBotlyNoob
Copy link
Author

I would like this primarily for parsing HTML-like languages (Svelte, JSX) that use curly brackets in attributes

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.

Could you add a documentation, when one could use new functions? Maybe mention a your use case, where this could be useful.

You should add tests for all possible inputs that you could imagine, failed cases also need to be tested.

As you introduces a parse state which stores expected open and close delimiters, probably that state could replace SingleQ and DoubleQ states. I would like if you try to do that generalization

@@ -17,7 +17,9 @@ include = ["src/*", "LICENSE-MIT.md", "README.md"]
document-features = { version = "0.2", optional = true }
encoding_rs = { version = "0.8", optional = true }
serde = { version = "1.0.100", optional = true }
tokio = { version = "1.0", optional = true, default-features = false, features = ["io-util"] }
tokio = { version = "1.0", optional = true, default-features = false, features = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert changes in that file, they are not related to the feature

Comment on lines +245 to +247
/// Returns an iterator over the attributes of this tag, with custom quotes.
pub fn attributes_with_custom_quotes(&self, custom_quotes: &'a [(u8, u8)]) -> Attributes {
Attributes::wrap(&self.buf, self.name_len, custom_quotes, false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This requires documentation for how you plan to use that method. For what you need an array of custom quotes? How user of this API should fill it? Is it not enough to have one opening and closing quote?

Copy link
Collaborator

@Mingun Mingun Jan 22, 2023

Choose a reason for hiding this comment

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

Ok, I understood the idea. custom_quotes is an array of possible pairs. I propose to use an F: Fn(u8) -> Option<u8> instead, which would map open delimiter to close delimiter or to None if the input is not an open delimiter. Then you should save the gotten close delimiter in the state and do check against it when you checking for a close delimiter.

You don't have to return Attributes here, you can return IterState (or a new wrapper around it if you find wrapper more ergonomic). Thus Attribute struct remains unchanged and you will get a way to get the attribute shape in your code.

Comment on lines +30 to +31
/// The quotes of the attribute value.
pub quote: Attr<()>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The rationale of this change escapes me. Why it is required? If you want to expose used delimiters, it is better just introduce open: u8 and close: u8 members (or the one tuple member).

Copy link
Collaborator

Choose a reason for hiding this comment

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

See that.

@Mingun
Copy link
Collaborator

Mingun commented Jan 22, 2023

Anyway, your should also add a point to the Changelog.md under New Features section.

@dralley
Copy link
Collaborator

dralley commented Jul 10, 2023

@TheBotlyNoob Do you intend to continue this work?

@TheBotlyNoob
Copy link
Author

I don't think so, sorry. I've gotten caught up in other projects. I seem to do that a lot :/.

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.

4 participants