Skip to content

Conversation

runemadsen
Copy link
Collaborator

Hi again :-)

The regexp sanitizer meant to remove comments also removes http://something, which results in broken JSON. This pull requests adds a breaking test, and fixes the regexp in order to fix the test.

  • Rune

@sideshowcoder
Copy link
Owner

Good catch!

I think it would make more sense to match on the first non-blank of the line to be // so. Any reason for the identifier of a valid comment to be //\s instead of \s*// ? maybe I am thinking wrong.

Thanks for all the work by the way!

@runemadsen
Copy link
Collaborator Author

You're more than welcome :-)

Ah, that makes a ton of sense. I'm not that good at regexes, so thanks for catching that. Do you want me to update the pull request, or do you want to do it?

Thanks.

@sideshowcoder
Copy link
Owner

Well how ever you want, you already added the test so if you want to go ahead just do it.

@runemadsen
Copy link
Collaborator Author

Actually, I do remember now why I did this. If we do \s*//, than it won't remove comments like this:

// my values
{
  "myvar" : "myval"
}

That doesn't matter much to me, but that type of root comment is in the tests already and my new test breaks when using \s*//. I guess that "zero or more whitespace" will also actually catch the http:// line too.

@sideshowcoder
Copy link
Owner

Yeah I guess your solution makes more sense than. I will merge as is, cool :)

sideshowcoder added a commit that referenced this pull request Nov 14, 2013
Support the use of // in http:// and URI's in JSON
@sideshowcoder sideshowcoder merged commit 7c5eba2 into sideshowcoder:master Nov 14, 2013
@sideshowcoder
Copy link
Owner

@runemadsen if you are interested I can give you commit access to the repo, since you seem really interested in canned.

@runemadsen
Copy link
Collaborator Author

Sure, that would be awesome. We're using it quite a bit now to fake some API dependencies in our tests, and will probably come across small fixes.

@sideshowcoder
Copy link
Owner

Here have some commit right ;)

sideshowcoder added a commit that referenced this pull request Nov 19, 2014
Support the use of // in http:// and URI's in JSON
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.

2 participants