Skip to content

Conversation

orgads
Copy link
Contributor

@orgads orgads commented Nov 7, 2019

If there is no space before the closing tag, and the tag is followed by text, validation failed:

<main><empty/>text</main>

Purpose / Goal

Type

Please mention the type of PR

  • [X]Bug Fix
  • [ ]Refactoring / Technology upgrade
  • [ ]New Feature

@coveralls
Copy link

coveralls commented Nov 7, 2019

Coverage Status

Coverage remained the same at 96.309% when pulling 9d0f04a on orgads:self-closing-with-text into 6a980ac on NaturalIntelligence:master.

@orgads orgads force-pushed the self-closing-with-text branch from bd0de67 to c7a0cad Compare November 7, 2019 15:43
@amitguptagwl
Copy link
Member

Thanks for raising this PR. LEt me go through the changes.

@orgads orgads force-pushed the self-closing-with-text branch 2 times, most recently from c37122e to e845e02 Compare November 8, 2019 05:29
@orgads
Copy link
Contributor Author

orgads commented Nov 8, 2019

Added another test. Would you prefer that on this case the error would be "Tag in/valid is an invalid name"?


it("should not consider this as self-closing tag", function() {
const xmlData = "<rootNode><in/valid></rootNode>";
const expected = {code: "InvalidAttr", msg: "attribute /valid has no space in starting."};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this expected to be InvalidTag, and "Tag in/valid is an invalid name"? Or are you ok with this error?

@orgads orgads force-pushed the self-closing-with-text branch from e845e02 to 0aaee2d Compare November 8, 2019 06:47
@orgads
Copy link
Contributor Author

orgads commented Nov 8, 2019

I changed this to behave just like <foo />. When I added tests I noticed some more bugs, so I opened separate PRs for them. If you want me to reorder the commits I can do it. Tests will need to be adapted on this case.

If there is no space before the closing tag, and the tag is followed
by text, validation failed:

<main><empty/>text</main>
@orgads orgads force-pushed the self-closing-with-text branch from 0aaee2d to 9d0f04a Compare November 8, 2019 08:59
expect(result).toEqual(expected);
});

/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is commented out because it doesn't pass due to another bug. This is addressed in #205

@orgads
Copy link
Contributor Author

orgads commented Nov 8, 2019

This fix is minor, so I reordered the commits to have it first. The other unicode PR now depends on this change.

@amitguptagwl
Copy link
Member

By inspiring your this fix, I've just pushed a commit. Please have a look and check if it solves the purpose of both PR. If not you can revise your PR.

@amitguptagwl
Copy link
Member

@orgads Do we need this PR anymore?

@orgads orgads closed this Nov 15, 2019
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.

3 participants