Closed runemadsen closed 10 years ago
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!
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.
Well how ever you want, you already added the test so if you want to go ahead just do it.
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.
Yeah I guess your solution makes more sense than. I will merge as is, cool :)
@runemadsen if you are interested I can give you commit access to the repo, since you seem really interested in canned.
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.
Here have some commit right ;)
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.