Support default schema when using multiple selector schemas

rcsandell commented 5 years ago

Pull request for #369 "Default Schema for No Matching Selector when using Multiple Schemas"

This adds support for having a base (default) schema while also using multiple selector schemas.

There are four behaviors for attaching schemas:

Attach schema without selector. This will extend the base schema and any attached selector schemas.
Attach schema with selector. If the given selector schema exists, extends the found schema with any new fields. Or add an additional schema that matches the selector only. The new selector schema will first be extended by the base schema, if the base schema is already specified.
Replace schema without selector. This replaces the base schema and removes any attached selector schemas. If you want to continue to use the original selector schemas on the collection they must be reattached.
Replace schema with selector. This replaces the schema with the same selector with the new schema fields extended by the base schema, if the base schema is already specified. Othewise adds the new selector schema if it was not found.

It is possible to use schemas on collections with or without the base schema specified, and with or without selector schemas specified. The selector schemas will always inherit the base schema. Replacing the base schema effectively serves as deleting all schemas off the collection.

Before I started the tests were producing error: "1) extending a schema with a selector after attaching it, collection2 validation respects the extension" and that error is still being produced.

rcsandell commented 5 years ago

Reference #369 @aldeed . Anything else you'd like to see with the commit?

rcsandell commented 5 years ago

Bump. Reference https://github.com/aldeed/meteor-collection2-core/issues/43

ramijarrar commented 5 years ago

@rcsandell I've rebased/squashed your PR against most recent changes and did some minor cleanup work in #392 (you are maintained as author). Hopefully this helps to potentially have this merged in sooner.

rcsandell commented 5 years ago

All I can see is you've deleted my description in the documentation of how the new attach schema functionality works?

ramijarrar commented 5 years ago

Hi @rcsandell - I resolved the conflicts with the latest version (3.0.2)

The following files were redundant / not relevant to the changes and have been removed:

.README.md.swp
package/collection2/.collection2.js.swp
tests/package-lock.json
tests/package.json
package-lock.json

I reworked the description you added originally as I found it very verbose describing the four scenarios separately given they are really just variations on the existing replace / merge behavior (IMO most people will already expect that selector schemas will extend an already attached schema and hence a brief note to clarify is clearer / causes less confusion).

Happy to close the PR in favor of this one if you want to bring it up to date yourself.

rcsandell commented 5 years ago

Thanks, I don't think it's obvious at all because it's new functionality that never existed before. Hence why it necessary to list all the options, and each of their specific behaviours.

I've removed the redundant files, and placed all the changes in a single commit.

ramijarrar commented 5 years ago

Okay - I can see you've resolved the conflicts so will close my PR accordingly. Will leave it to @aldeed to document as he feels needed. Thanks for your work on this!

rcsandell commented 5 years ago

No worries dude, I'm sure @aldeed will also have a list of changes

rcsandell commented 5 years ago

Bump, pull request for "help wanted" feature https://github.com/aldeed/meteor-collection2-core/issues/43 @aldeed @serkandurusoy @jgladch

serkandurusoy commented 5 years ago

@rcsandell I am not a maintainer of this reposiytory :/

red-meadow commented 5 years ago

This would be an excellent feature.

rcsandell commented 5 years ago

@aldeed @mquandalle @newsiberian @wI2L @jimmiebtlr @dmitrijs-balcers @yogiben @spencern @serkandurusoy @raix @pierreozoux @phocks @petrometro @mryraghi @luckyyang @lourd @grabbou @dandv @czeslaaw @boustanihani @FatBoyXPC

Hey Previous contributors,

Has Eric Dobbertin and Collection2 died?

What will it take to get the show on the road?

Kind Regards,

FatBoyXPC commented 5 years ago

I haven't used meteor in a few years, sorry @rcsandell!

luckyyang commented 5 years ago

Sorry I can not answer your question.

raix commented 5 years ago

@rcsandell thats a pretty massiv mention - I haven’t used Meteor the past couple of years - I know Eric is very low on time so I’d expect that to be the reason.

That said this comment doesn’t bring you closer to solving the actual problem of getting this pr merged and released - for that I’m sorry

Kind regards Morten

rcsandell commented 5 years ago

Hi Eric,

Thanks for getting back to us on this one. I'll reply to your comments individually.

rcsandell commented 5 years ago

Thanks for your review Eric.

I've made comments on the comments, it seems like the only task is to remove line 115 from the new commit. If that is all then I will go ahead and make the commit. Otherwise if there is anything else I'd like to include it before making the next commit.

Kind Regards, Robert

harryadel commented 4 years ago

Hello @rcsandell, just dropping in to let you know that we're currently working our way through the PRs and will get to yours eventually.

harryadel commented 4 years ago

This is really good, thank you @rcsandell. Though, have you tried to run the test command? Also, it'd be super good if new test cases are added to account for the changes. For instance validating the removal of all selector schemas if we were to update the base schema with replace being set.

Meteor-Community-Packages / meteor-collection2

Support default schema when using multiple selector schemas #391