clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
490 stars 110 forks source link

Add in a better pluralise module #48

Closed M1chaelTran closed 7 years ago

M1chaelTran commented 7 years ago

Could we replace this https://github.com/clarkie/dynogels/blob/master/lib/index.js#L70 with a real pluralism library like this? https://github.com/blakeembrey/pluralize?

Hate how it creates table name like 'Category' with Categorys!

If we going to do something, do it right. Otherwise, remove it!

It would help with this user: https://github.com/ryanfitz/vogels/issues/37

AaronHarris commented 7 years ago

I don't necessarily think this will solve those people's problems, as their was an issue of when/where tablename is specified and when a table gets created. At the risk of adding more dependencies, while not solving the root cause of those people's issues, I think the documentation should be updated to clarify when and where tablename should be updated.

While I think simply adding 's' to the end is kind of weak and should probably be removed in favor of just using the model name for the table name, I can't say its worth all the breakage that would occur for existing applications.

clarkie commented 7 years ago

I've never liked the 's' either which is why all our model definitions look like this:

const productTable = dynogels.define('product', {
  hashKey: 'id',
  timestamps: true,
  schema: schema,
  indexes: indexes,
  tableName: helpers.generateTableName('product'),
});

where

generateTableName = (name) => `${config.dbPrefix}_${name}

I disagree that removing the s would be a problem as long as it came with a major version bump and work around similar to our usage

iliasbhal commented 7 years ago

I think AaronHarris is right, it would be much better to just drop this functionality. And just let people choose the way they want their tables to be named.

I personally really dislike this type of imposed functionality, so I just wrapped 'dynogels.define' in a function to do this automatically ( prefixing, etc )

clarkie commented 7 years ago

I'd happily accept a PR that removes this. It'll be a major bump though. Cheers