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

Added check for ignoreCase flag on RegExp queries. #26

Merged
merged 3 commits into from
Sep 8, 2016
Merged

Added check for ignoreCase flag on RegExp queries. #26

merged 3 commits into from
Sep 8, 2016

Conversation

jftanner
Copy link

@jftanner jftanner commented Aug 31, 2016

This is a fix for #25.
Cloudant.prototype.buildSelector now parses the ignoreCase from RegExp objects and provides it to the $regex operator in PCRE format by prepending (?i) to the expression.

@slnode
Copy link

slnode commented Aug 31, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode
Copy link

slnode commented Aug 31, 2016

Can one of the admins verify this patch?

@jftanner
Copy link
Author

I have now signed the CLA.

@ghost
Copy link

ghost commented Aug 31, 2016

Correction: now I've signed the CLA. It would help if I paid attention to which account I'm signed in on.

@superkhau
Copy link
Contributor

Can you add some tests to verify your changes and prevent regressions in the future?

@ghost
Copy link

ghost commented Aug 31, 2016

I'd be more than happy to. Unfortunately, I can't start the tests locally.

Running npm test starts linters that have existing errors. Starting the tests manually with make test errors with: Error: Cannot initialize connector {}: Invalid settings: "url" OR "username" AND "password" required

I'd guess I'm missing a private configuration file/environment variables.

If you can provide instructions to run the tests, I'd be happy to add a new one. (Feel free to reach me via Sametime, if necessary.)

Edit: Looking at /test/init.js I see it's looking for cloudant authentication from the environment. Does the database have to have any certain configuration, or will a blank one do?

Disregard. I've figured out how to get regexp.test.js running. I'll add a test there.

@ghost
Copy link

ghost commented Sep 1, 2016

I added the test yesterday, btw, in case Github didn't send a notification on that.

@superkhau
Copy link
Contributor

@slnode test please

@superkhau
Copy link
Contributor

@jftanner Status update: working on some CI issues to get greens before merging. Thanks for your patience.

@superkhau
Copy link
Contributor

The patch LGTM FYI. ;)

@superkhau
Copy link
Contributor

Foo.find({where: {bar: {regexp: '^b'}}}, function(err, entries) {
console.log (entries);
entries.should.have.lengthOf(1);
entries[0].bar.should.equal('b');
done();
});
});
it('find all foos that are case-insensitive B', function(done) {
Foo.find({where: {bar: {regexp: '/B/i'}}}, function(err, entries) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (err) return done err;

@superkhau
Copy link
Contributor

Actually @TannDev, can you address the two new comments? Didn't notice them till now.

@ghost
Copy link

ghost commented Sep 8, 2016

@superkhau While I agree with both your comments, my added test matches the style of all the other tests in that file. Would you like to me change all the tests to reflect your comments?

@superkhau
Copy link
Contributor

While I agree with both your comments, my added test matches the style of all the other tests in that file. Would you like to me change all the tests to reflect your comments?

You are right in following the conventions outlined for consistency. The issue is that they are wrong to begin with. Please do not worry about it, @jannyHou has another PR that addresses the same issues anyways so we can fix it internally there. Thanks for the contribution, gonna merge.

@jannyHou Can you remove the console logs here too later after you rebase? I forgot the PR, but it was the ESLint one I reviewed last time.

@superkhau superkhau merged commit 8cf892c into loopbackio:master Sep 8, 2016
@superkhau
Copy link
Contributor

Merging because CI for Cloudant isn't working properly anyways, no point waiting for greens.

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.

None yet

4 participants