search

geddy / model

Datastore-agnostic ORM in JavaScript
265 stars 55 forks source link

Mongo is the name of the adapter, but docs and code say mongodb #198

Open OscarGodson opened 10 years ago

OscarGodson commented 10 years ago

Except for here: https://github.com/geddy/model/blob/master/lib/adapters/mongo/index.js#L64

Everywhere it says "mongodb". This is confusing. Should the fix be changing this.name to be mongodb or should the docs just reflect that its mongo not mongodb like it is currently?

ben-ng commented 10 years ago

I think we should fix the docs, since changing the name would be a breaking change. What do you think @mde?

OscarGodson commented 10 years ago

@ben-ng Yeah, I have no preference either way, but couldn't the mongo adapter just require the mongodb adapter basically and just run that but have a deprecation notice for awhile?

mde commented 10 years ago

I wrote code in there to alias names, particularly postgres, because I know people (including me) will fuck it up in their config:

https://github.com/geddy/model/blob/master/lib/adapters/index.js#L7

I don't have a strong preference about renaming the internal 'name' property for the Mongo adapter -- it's not really documented anywhere. But 'postgres' is a shortened name too.

At bare min we should add 'mongodb' to the alias list in case people use that in their config. But it might be better to document the 'name' property for each of the adapters.

OscarGodson commented 10 years ago

Oh, rad, you have aliasing already. Maybe just a doc update and add an alias

mde commented 10 years ago

How about a PR? :D

OscarGodson commented 10 years ago

Fixed it here: https://github.com/geddy/model/pull/203