fixing string replace

[vshesh]

I tried using string replace on a fresh install of wisp today. The code seems to be broken, unless I'm misunderstanding something here - why is there a variable 's' in this function?

I fixed it to the best of my knowledge, and the playground compiles it to

var replace = exports.replace = function replace(string, match, replacement) {
        return isString(match) ? string.replace(new RegExp(patternEscape(match), 'g'), replacement) : isRePattern(match) ? string.replace(new RegExp(match.source, 'g'), replacement) : 'else' ? (function () {
            throw '' + 'Invalid match arg: ' + match;
        })() : void 0;
    };

which looks right to me. I had to actually manually fix this in the string.js file, so if you could update the npm package as well that would be awesome.

[Gozala]

I tried using string replace on a fresh install of wisp today. The code seems to be broken, unless I'm misunderstanding something here - why is there a variable 's' in this function?

@vshesh thanks for catching this error, most likely I introduced this regrassion during refactoring. I think string parameter usede to be called s before. Changes look good, could you please add at least one basic tests to ensure no further regressions ?

If you aren't willing to work on adding tests that's also ok, please let me know if that is a case and I'll take over the pull.

Thanks again

[vshesh]

I'm not entirely familiar with the testing harness (been having some namespacing issues) but I gave it a shot. Lmk if there is anything you'd like me to change.

[Gozala]

I'm not entirely familiar with the testing harness (been having some namespacing issues) but I gave it a shot.

Let me know if you still have issues with namespaces & I'll try to help you figure those out.

Lmk if there is anything you'd like me to change.

Changes themself look good.

You should be able to add tests for replace here:

https://github.com/Gozala/wisp/blob/master/test/string.wisp

You could likely also borrow them from clojurescript repo, which I have benig doing for a lot of tests ;)

[vshesh]

Ok I put in some tests - if you'd like more let me know.

[vshesh]

Argh, I'm sorry. It should be fixed now.

[Gozala]

Thanks merged! Looks like I broke a tree by merging submitted pull requests. I'll publish to npm once I fix it and tests will start pass on travis.