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

Issue #581 - coerce attribute names to lower case in HTML mode #3306

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

davidfei222
Copy link

Fix for issue #581. Attribute names will now always be coerced to lower case when attempting to get or set attributes to keep the behavior in line with how cheerio.load() handles attribute names.

@walshyb
Copy link

walshyb commented Jul 28, 2023

This PR updates Cheerio's .attr() functionality to more closely match jQuery's .attr(), which saves and retrieves attributes by lowercasing the input name.

@rjpatt
Copy link

rjpatt commented Jul 28, 2023

Adjusted a few existing tests to be able to support tests for this fix.

@fb55
Copy link
Member

fb55 commented Jul 29, 2023

Thanks a lot for this PR! Some notes:

  1. The change in the fixture led to a need for quite a few unrelated changes. Please use one of the other fixtures or add a new one.
  2. Cheerio has an XML mode for working with XML. XML is case-sensitive, so we don't want to lowercase tag names in XML. You will probably end up with something along the lines of
attrNameToUse = this.options.xmlMode ? attrName : attrName.toLowerCase()

@davidfei222
Copy link
Author

@fb55 Apologies for the late response - my colleagues and I have been very busy at work recently. The requested changes have been made - please re-review at your earliest convenience. Thanks!

@davidfei222 davidfei222 changed the title Issue #581 - coerce attribute names to lower case Issue #581 - coerce attribute names to lower case in HTML mode Aug 21, 2023
Copy link

@CyberFlameGO CyberFlameGO left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants