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

Revert "Add lowercase of åäö in entityMap" #84

Merged
merged 1 commit into from Jul 3, 2020
Merged

Revert "Add lowercase of åäö in entityMap" #84

merged 1 commit into from Jul 3, 2020

Conversation

joestringer
Copy link
Contributor

This reverts commit ef20262.

These characters were already included lower in the file in the sorted
order for lower-case runes.

Including duplicates like this causes issues in Rhino 1.7.12:

Property "auml" already defined in this object literal.

Fixes: #83

@brodybits brodybits added the bug Something isn't working label Jul 3, 2020
@brodybits
Copy link
Member

Thanks! I may need some time to double-check before merging.

I suspect that the issue could have been avoided had we started using using eslint or a derivative such as standard, as discussed in some other issues.

Copy link
Member

@karfau karfau left a comment

Choose a reason for hiding this comment

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

Awesome finding!
Just checked, the removed keys are existing below.

@brodybits
Copy link
Member

This reverts commit ef20262.

(introduced by PR #23)

I have some nits coming up.

lib/entities.js Outdated Show resolved Hide resolved
This reverts commit ef20262.

These characters were already included lower in the file in the sorted
order for lower-case runes.

Including duplicates like this causes issues in Rhino 1.7.12:

    Property "auml" already defined in this object literal.

Fixes: #83
@brodybits brodybits merged commit 8f02868 into xmldom:master Jul 3, 2020
@brodybits
Copy link
Member

Thanks for the contribution. I do have a few more nits for the future:

  • The test coverage of lib/entities.js is very spotty. I just raised PR test European HTML entities #86 to start testing the affected entities, to gain some extra confidence, and it does look good with this change.
  • I noticed that the change was contributed from the master branch of the fork. It is generally recommended to contribute changes from another, temporary branch instead, to avoid likely divergence. PRs are generally merged as squash commits in this project.

As I had implied before, it is not very nice when a revert would undo formatting cleanup. But I think taking it out would make it not a truly complete revert. I think PR #85 solved it by simply getting rid of the useless line.

Once we have better test coverage, Stryker should show us if we have duplicated string entities again.

On a positive note: kudos for putting the information into the revert commit itself. This should make it easier for people to understand the rationale without having to dig through GitHub.

Thanks again for the contribution!

brodybits added a commit that referenced this pull request Jul 3, 2020
to verify the functionality related to 8f02868
(PR #84) which reverts commit ef20262
(introduced by PR #23)

Co-authored-by: Christopher J. Brody <chris.brody+brodybits@gmail.com>
Co-authored-by: Christian Bewernitz <coder@karfau.de>
This was referenced Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate inclusion of entities causes execution failure in Rhino
3 participants