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

Validator: Fail for stray ampersand characters #215

Merged
merged 2 commits into from
Dec 24, 2019

Conversation

orgads
Copy link
Contributor

@orgads orgads commented Dec 21, 2019

Fixes #214

Type

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

@coveralls
Copy link

coveralls commented Dec 21, 2019

Coverage Status

Coverage increased (+0.02%) to 96.445% when pulling d36575f on orgads:ampersand into df830dc on NaturalIntelligence:master.

@amitguptagwl
Copy link
Member

Thanks for your PR @orgads

I can see that after this PR we'll be doing 3 more checks per character in the given data which will impact the performance. Hence, I'm looking for options to minimize it. Eg when we find & we can check if the consecutive letters are word letter until we find ;. if this sequence is not found we can throw an error. It'll save 2 comparisons per character.

Let me know your opinion on this.

@orgads
Copy link
Contributor Author

orgads commented Dec 22, 2019

Done, but I still need to add some unit tests.

The following:
    const xmlData = '<name length="bar" length="baz"></name>';
    var expected = { code: 'InvalidAttr', msg: "Attribute 'length' is repeated.", line: 1 }
    var result = validator.validate(xmlData).err;
    expect(result).toEqual(expected);

Can be replaced by:
    validate('<name length="bar" length="baz"></name>', { InvalidAttr: "Attribute 'length' is repeated." }
@orgads orgads changed the title Validator: Fail for orphan ampersand characters Validator: Fail for stray ampersand characters Dec 22, 2019
@amitguptagwl
Copy link
Member

@orgads Let me know once you're done.

@orgads
Copy link
Contributor Author

orgads commented Dec 23, 2019

I'm done

@amitguptagwl
Copy link
Member

Thanks for the effort you spent.

@amitguptagwl amitguptagwl merged commit a8ccdb9 into NaturalIntelligence:master Dec 24, 2019
@orgads
Copy link
Contributor Author

orgads commented Dec 24, 2019

You're welcome. What about my other PR?

@orgads orgads deleted the ampersand branch December 24, 2019 12:40
@amitguptagwl
Copy link
Member

That PR needs more time to review. As I'm out of the station for 1 more week, I'm not able to finish it. Will do it hopefull by 5th Jan.

@orgads
Copy link
Contributor Author

orgads commented Dec 27, 2019

Sure, take your time :)

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.

Validation of unescaped ampersand
3 participants