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 testcase and fix for attributes deduplication in form and empty elements #1950

Merged
merged 5 commits into from
May 8, 2023

Conversation

perlan
Copy link
Contributor

@perlan perlan commented May 5, 2023

This is an attempt to handle deduplication of attributes in form-elements and should fix #1949

Copy link
Owner

@jhy jhy left a comment

Choose a reason for hiding this comment

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

Looks good, would like to get the duplicate code factored out.

@jhy
Copy link
Owner

jhy commented May 6, 2023

(BTW, I am happy to make that change myself if you prefer, LMK.)

@perlan
Copy link
Contributor Author

perlan commented May 6, 2023

Thank you for responding to my PR. I took a quick look at the code path and I'm not familiar enough with the code to try any major refactoring but I can certainly factor out the deduplication code.

perlan added 3 commits May 6, 2023 17:20
Also add new testcase and fix for attributes deduplication in empty tags
To use spaces instead of tabs as rest of the file
@perlan perlan changed the title Add testcase and fix for form attributes deduplication Add testcase and fix for attributes deduplication in form and empty elements May 6, 2023
@perlan
Copy link
Contributor Author

perlan commented May 6, 2023

When looking at the code path, I realized that deduplication was not applied for empty tags either so I added another test case and fix for empty tags as well.

@perlan perlan requested a review from jhy May 6, 2023 16:40
Copy link
Owner

@jhy jhy left a comment

Choose a reason for hiding this comment

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

Thanks, looks great. I added a commit that makes the test case parameterized, to remove some duplication.

@jhy jhy merged commit 401c8b0 into jhy:master May 8, 2023
12 checks passed
@jhy
Copy link
Owner

jhy commented May 8, 2023

Thanks, merged!

@jhy jhy added this to the 1.16.2 milestone May 8, 2023
jhy added a commit that referenced this pull request May 8, 2023
@perlan perlan deleted the fix-1949 branch May 8, 2023 09:04
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.

Attributes in form-elements are not deduplicated when parsing as HTML
2 participants