Fir for Add sparse options for index #11

ramakrishnan · 2016-10-03T09:16:20Z

Fix for #10 Added provision to pass sparse option while creating index

wzrdtales

Any change must be backwards compatible. Please ensure to not modify the API in a breaking manner, but feel free to extend the existing interface.

ramakrishnan · 2016-10-04T05:49:17Z

@wzrdtales I feel passing the index options as an object will make the code more flexible. I will add a work around for backward compatibility. Moving forward shall we encourage users to pass as options?

Please share your thoughts.

wzrdtales · 2016-10-04T07:39:09Z

@ramakrishnan I would rather go for an options object than adding the huge bunch of arguments that you want to add. And it would be the better solution if we talk about flexibility though.

Though from my side I would remove your params again and give the optionality to pass an object in instead and handle the backwards compatibility by just checking if the second param is a string or an object.

wzrdtales · 2016-10-04T07:40:52Z

@ramakrishnan Confused your changes with the original, but it keeps the same. Just passing in the second param as an object should be enough to check for the original behavior.

ramakrishnan · 2016-10-04T07:42:34Z

Exactly in line with my thoughts. Backward compatibility I mean is to check if the second param is a string or an object.

wzrdtales

DocTag changes are not in place yet.

wzrdtales · 2016-10-04T12:15:06Z

index.js

@@ -158,13 +158,26 @@ var MongodbDriver = Base.extend({
   * @param columns         - The columns to add an index on
   * @param	unique          - A boolean whether this creates a unique index


Please also adjust the tags properly.

ramakrishnan

Corrected param tags

wzrdtales · 2016-10-11T17:24:43Z

test/mongodb_test.js

@@ -230,7 +230,7 @@ vows.describe('mongodb').addBatch({
          return this.callback(err);
        }

-        db.addIndex('event', 'event_title', 'title', false, function(err, data) {


Last thing missing here: You have deleted the old tests, but to actually cover the old and new functionality, the old and the new tests have to coexist, rather than one gets rewritten.

But these are internal calls made within the test case hence I changed them to use the updates style. I have also added a separate test case for addIndex for the previous style.

Sorry, this wasn't clear from the diff. Please don't adjust tests in the future and instead add directly new ones instead :)

wzrdtales · 2016-10-11T17:24:56Z

test/mongodb_test.js

@@ -299,7 +299,7 @@ vows.describe('mongodb').addBatch({
          return this.callback(err);
        }

-        db.addIndex('event', 'event_title', 'title', false, function(err, data) {


wzrdtales · 2016-10-11T17:24:59Z

test/mongodb_test.js

@@ -122,7 +122,7 @@ vows.describe('mongodb').addBatch({
          return this.callback(err);
        }

-        db.addIndex('event', 'event_title', 'title', false, this.callback);


wzrdtales · 2016-10-13T10:03:42Z

index.js

-    };
+  addIndex: function(collectionName, indexOptions, callback) {
+    var options = {};
+    if (indexOptions.constructor.name == 'Object') {


Is there any reason for not using typeof?

Also please always use type safe comparisions and only use non type safe where you really need to fallback.

typeof [] is Object and typeof {} is also Object. Hence constructor.name

This is not enough of a reason for me though. [] in javascript is an object just as {} is, the only difference are some specifics, but even with those an array is still an object in javascript, though.

Agree, but for this case we expect the options to be of type {}

I still would go for typeof instead, or can you call out a case where a passed array would result in an unwanted behavior?

wzrdtales · 2016-10-13T10:04:43Z

index.js

+        columns: indexOptions.columns,
+        unique: indexOptions.unique
+      };
+      if (indexOptions.hasOwnProperty('sparse')) {


What would be the benefit over indexOptions.sparse here? As what I can see here, it is just a simple object being passed.

indexOptions.sparse can only be true or false, this option will be considered only if indexOption contains a property sparse. As if(false) will be false

Ok, lets be a bit more specific: What is the default value of sparse? If it is false, then it can just be omitted. If the default is true, well then please check for indexOptions.sparse === false instead.

sparse itself is optional parameter for addIndex, hence we should use the value as set by user. Hence the presence of sparse is checked before passing them to createIndex call.

But sparse itself does only accept true or false itself. So I would rather check for the non default value in a type safe manner instead of doing hasOwnProperty.

https://docs.mongodb.com/manual/core/index-sparse/#create-a-sparse-index

As false is the default, we can just safely omit it in the case of the value being non true.

wzrdtales · 2016-10-13T10:08:35Z

index.js

+        options.sparse = indexOptions.sparse
+      }
+    } else {
+      log.warn('addIndex can now accept Object for passing index Options.');


Please remove that log, this is an extension of the API not a deprecation though.

To add, this information is more a target for documentation rather than adding some logs here :)

That piece of information will go here https://github.com/db-migrate/english-docs/blob/master/API/NoSQL.md#addindexcollectionname-indexname-columns-unique-callback

wzrdtales · 2016-10-13T10:09:33Z

Apart from the last mentioned items everything looks fine though :)

ramakrishnan added 2 commits October 3, 2016 12:55

Merge remote-tracking branch 'upstream/master'

79f2fdc

Fix for passing sparse options for createIndex

f6e47e7

ramakrishnan changed the title ~~Fir for #10 Add sparse options for index~~ Fir for Add sparse options for index Oct 3, 2016

wzrdtales requested changes Oct 3, 2016

View reviewed changes

Fix for addIndex backward compatibility

ce222c6

wzrdtales requested changes Oct 4, 2016

View reviewed changes

Adjusted @params tags

7e79705

ramakrishnan commented Oct 11, 2016

View reviewed changes

wzrdtales approved these changes Oct 11, 2016

View reviewed changes

wzrdtales requested changes Oct 11, 2016

View reviewed changes

wzrdtales approved these changes Oct 13, 2016

View reviewed changes

wzrdtales reviewed Oct 13, 2016

View reviewed changes

ramakrishnan added 2 commits October 13, 2016 15:49

Review comments

7b0f4ad

Remove warning log

f71fdc3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fir for Add sparse options for index #11

Fir for Add sparse options for index #11

ramakrishnan commented Oct 3, 2016 •

edited

Loading

wzrdtales left a comment

ramakrishnan commented Oct 4, 2016

wzrdtales commented Oct 4, 2016

wzrdtales commented Oct 4, 2016

ramakrishnan commented Oct 4, 2016

wzrdtales left a comment

wzrdtales Oct 4, 2016

ramakrishnan left a comment

wzrdtales Oct 11, 2016

ramakrishnan Oct 12, 2016

wzrdtales Oct 13, 2016

wzrdtales Oct 11, 2016

wzrdtales Oct 11, 2016

wzrdtales Oct 13, 2016

wzrdtales Oct 13, 2016

ramakrishnan Oct 13, 2016

wzrdtales Oct 13, 2016

ramakrishnan Oct 13, 2016

wzrdtales Oct 13, 2016

wzrdtales Oct 13, 2016

ramakrishnan Oct 13, 2016

wzrdtales Oct 13, 2016

ramakrishnan Oct 13, 2016

wzrdtales Oct 13, 2016

wzrdtales Oct 13, 2016

wzrdtales Oct 13, 2016

wzrdtales commented Oct 13, 2016

		@@ -158,13 +158,26 @@ var MongodbDriver = Base.extend({
		* @param columns - The columns to add an index on
		* @param unique - A boolean whether this creates a unique index

Fir for Add sparse options for index #11

Are you sure you want to change the base?

Fir for Add sparse options for index #11

Conversation

ramakrishnan commented Oct 3, 2016 • edited Loading

wzrdtales left a comment

Choose a reason for hiding this comment

ramakrishnan commented Oct 4, 2016

wzrdtales commented Oct 4, 2016

wzrdtales commented Oct 4, 2016

ramakrishnan commented Oct 4, 2016

wzrdtales left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramakrishnan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzrdtales commented Oct 13, 2016

ramakrishnan commented Oct 3, 2016 •

edited

Loading