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

Implement $min, $max, $currentDate modifiers #7858

Merged
merged 3 commits into from Nov 8, 2016

Conversation

sebakerckhof
Copy link
Contributor

Related issue: #7857

This implements $min, $max and partially $currentDate + tests

I didn't implement $mul, since there can be numeric type conversions which I don't know how to deal with in JS. There's also no support for timestamp type in $currentDate, since from what I understand of the docs, this is an internal type I can't generate myself.

@@ -2086,7 +2086,12 @@ Tinytest.add("minimongo - modify", function (test) {
coll.update(query, mod);
var actual = coll.findOne();
delete actual._id; // added by insert
test.equal(actual, expected, EJSON.stringify({input: doc, mod: mod}));

if(typeof expected === "function"){
Copy link
Contributor

Choose a reason for hiding this comment

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

Style? Spaces around (, ).

if(arg.$type !== "date"){
throw MinimongoError("Minimongo does currently only support the date type in $currentDate modifiers");
}
}else if(arg !== true){
Copy link
Contributor

Choose a reason for hiding this comment

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

Style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a style guide somewhere? The only reference I could find in the contributing.md refers to: https://guide.meteor.com/code-style.html#javascript which doesn't say a lot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But most of the code doesn't seem to match that guide? Anyway I added a commit to make it along the lines of the other code nearby.

Copy link
Contributor

Choose a reason for hiding this comment

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

:-)

This is why I try to just match the style of the code around it.

@abernix
Copy link
Contributor

abernix commented Oct 18, 2016

@sebakerckhof Could you rebase this off devel? I believe it should solve the failing tests.

@sebakerckhof
Copy link
Contributor Author

@abernix done.

This would probably need to get an entry in history.md, but I don't know yet under which section it will come.

@laosb
Copy link
Contributor

laosb commented Oct 20, 2016

@sebakerckhof In most cases you should just put your entry in "v.NEXT" section :)

if (typeof arg === "object" && arg.hasOwnProperty("$type")) {
if (arg.$type !== "date")
throw MinimongoError("Minimongo does currently only support the date type in $currentDate modifiers");
} else if(arg !== true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Spaces around () please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They already have spaces, or which ones are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

} else if(arg !== true) {

->

} else if (arg !== true) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I missed that one.

target[field] = new Date();
},
$min: function (target, field, arg) {
if (typeof arg !== "number")
Copy link
Contributor

Choose a reason for hiding this comment

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

We use {} for if statements always (or we should anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, but that was copied from the already existing $inc function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh really? Sigh ;)

@sebakerckhof
Copy link
Contributor Author

Fixed the styling. No problem if you're being strict on styling, but if you want to open up more to contributors it would be good if there was an official styling guide and if MDG also followed that.

@tmeasday
Copy link
Contributor

tmeasday commented Nov 1, 2016

Yeah it's pretty tricky because the code base is pretty large and I guess the styling people used changed over the years. I think following the file the code is in is pretty reasonable.

@abernix
Copy link
Contributor

abernix commented Nov 2, 2016

@sebakerckhof Re: Style. As @mitar pointed out earlier in this issue, the Contributing.md links (somewhat annoyingly, via an extra click) to https://guide.meteor.com/code-style.html#style-guide says that Airbnb style is recommended. In my experience, MDG doesn't request changes to PRs which match Airbnb style and new code that is being added does usually match up with that. But the Meteor code base is far too big to go through and change all the older code that (mostly) was written before there was any such thing as a popular style guide for JavaScript. It would be risky, bug-introducing, and destroy a lot of Git-blame history to change it all at once, as glorious as that might be. If there is anything that can be done to make this more clear (like updating Contributing.md), a PR is likely welcome for that.

@tmeasday
Copy link
Contributor

tmeasday commented Nov 8, 2016

Thanks @sebakerckhof!

@tmeasday tmeasday merged commit ad54a10 into meteor:devel Nov 8, 2016
tmeasday added a commit that referenced this pull request Nov 8, 2016
benjamn pushed a commit that referenced this pull request Nov 11, 2016
benjamn pushed a commit that referenced this pull request Nov 14, 2016
sebakerckhof added a commit to sebakerckhof/meteor-collection-hooks that referenced this pull request Jan 3, 2017
New modifiers were added in Meteor 1.4.2, see: meteor/meteor#7858
zimme pushed a commit to zimme/meteor-collection-hooks that referenced this pull request Feb 4, 2017
New modifiers were added in Meteor 1.4.2, see: meteor/meteor#7858
},
$min: function (target, field, arg) {
if (typeof arg !== "number")
throw MinimongoError("Modifier $min allowed for numbers only");

Choose a reason for hiding this comment

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

Before I open an issue on this: Is there a reason why the operator isn't allowed for dates?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supported in MongoDB? Interesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no specific reason, I wasn't aware it could be used for dates. I'll create a follow-up PR.

Choose a reason for hiding this comment

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

Great, thanks 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into this a bit further and apparently $min and $max can be used to compare any types, where BSON type order is applied. So there's quite a bit of work involved to be fully compliant.

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

6 participants