Skip to content

Refix #210#224

Merged
dr-dimitru merged 3 commits intoveliovgroup:devfrom
exKAZUu:dev
Oct 12, 2016
Merged

Refix #210#224
dr-dimitru merged 3 commits intoveliovgroup:devfrom
exKAZUu:dev

Conversation

@exKAZUu
Copy link
Copy Markdown
Contributor

@exKAZUu exKAZUu commented Sep 8, 2016

#210

I've conducted the following tests and confirmed that there is no inconsistency.

@dr-dimitru
Copy link
Copy Markdown
Member

Are you sure it's good idea to remove checks?

@exKAZUu
Copy link
Copy Markdown
Contributor Author

exKAZUu commented Sep 9, 2016

Actually, I think we should keep checks even if some inconsistent behaviour exists because checks' errors are useful for debugging.
In my use case, I'd like to expect that find/findOne(), find/findOne({}), find/findOne(null), find/findOne(undefined) works as well as MongoCollection.
How do you think adding the following code in find and findOne:

check selector, Match.Optional Match.OneOf Object, String, null
check options, Match.Optional Object

Meteor API document says that the type of selector is Mongo Selector, Object ID, or String. So I think we should accept only Object, String, null and undefined.

Adding the above code yields the following result.

Code FilesCollection MongoCollection
find() FilesCursor LocalCollection.Cursor
find({}) FilesCursor LocalCollection.Cursor
find(null) FilesCursor LocalCollection.Cursor
find(undefined) FilesCursor LocalCollection.Cursor
find(false) FilesCursor LocalCollection.Cursor
find(true) Uncaught TypeError: Cannot use 'in' operator Uncaught TypeError: Cannot use 'in' operator
find([]) Exception: Match.Failed Uncaught Error: Invalid selector
find(12345) Exception: Match.Failed LocalCollection.Cursor
find('abcde') FilesCursor LocalCollection.Cursor
findOne() a document a document
findOne({}) a document a document
findOne(null) undefined undefined
findOne(undefined) undefined undefined
findOne(false) undefined undefined
findOne(true) Uncaught TypeError: Cannot use 'in' operator Uncaught TypeError: Cannot use 'in' operator
findOne([]) Exception: Match.Failed Uncaught Error: Invalid selector
findOne(12345) Exception: Match.Failed undefined
findOne('abcde') undefined undefined

@exKAZUu
Copy link
Copy Markdown
Contributor Author

exKAZUu commented Sep 9, 2016

I've added check statements in this PR.

files.coffee Outdated
find: (selector, options) ->
console.info "[FilesCollection] [find(#{JSON.stringify(selector)}, #{JSON.stringify(options)})]" if @debug
check selector, Match.OneOf Object, String, null
check selector, Match.Optional Match.OneOf Object, String, null
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@exKAZUu have you run all test after this changes?
Looks like it will fail on Boolean values

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Meteor API document says that the type of selector is Mongo Selector, Object ID, or String.
So I think we should accept only Object, String, null and undefined.

Should we accept Boolean values?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Neither theirs MongoCollection implementation is not fits the docs.
I think we should implement 1:1 behaviour

@exKAZUu
Copy link
Copy Markdown
Contributor Author

exKAZUu commented Sep 9, 2016

Thank you for your comment.
I added a new commit and updated the comparison table #224 (comment)

@exKAZUu
Copy link
Copy Markdown
Contributor Author

exKAZUu commented Oct 11, 2016

@dr-dimitru If you want me to fix something, could you tell me it?

Please note that we cannot satisfy with both check and the complete consistency with Meteor's find and findOne.
Personally, I'd like to keep check and break little consistency, but I'd like to pass null or undefined to FilesCollection.find and FilesCollection.findOne as well as Meteor's ones in order to avoid null and undefined checks.

@dr-dimitru
Copy link
Copy Markdown
Member

@exKAZUu everything looks good to me, working on the new release now.
Your PR will be merged

@exKAZUu
Copy link
Copy Markdown
Contributor Author

exKAZUu commented Oct 11, 2016

I see. Thanks!

@dr-dimitru dr-dimitru merged commit 41190ac into veliovgroup:dev Oct 12, 2016
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.

2 participants