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

AS-695: convert database to snake case #87

Merged
merged 7 commits into from
Oct 8, 2018

Conversation

wheelsandcogs
Copy link
Contributor

@wheelsandcogs wheelsandcogs commented Oct 8, 2018

  • All models should now exclusively use camelCase for property names.
  • All database tables should now exclusively have snake_case column names.

The mapping is performed by knexSnakeCaseMappers, for more information see https://vincit.github.io/objection.js/#snake-case-to-camel-case-conversion.

knexfile.js Outdated
if (process.env.SNAKE_MAPPER) {
test = { ...test, ...knexSnakeCaseMappers() };
development = { ...development, ...knexSnakeCaseMappers() };
}
Copy link
Contributor

@joefitter joefitter Oct 8, 2018

Choose a reason for hiding this comment

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

Won't this result in the case mappers always being applied? I was thinking more something like:

const mappers = process.env.SNAKE_MAPPER ? knexSnakeCaseMappers() : {};

module.exports = {
  test: {
    ...mappers,
    [settings]
  },
  development: {
    ...mappers,
    [settings]
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, yours does exactly the same thing in a slightly more verbose way. I didn't see the last 2 lines 🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... well in this case you wouldn't set SNAKE_MAPPER env var when running the migration command, but I've added it to the npm script for seed?

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 I can use your syntax to tidy it up.

@@ -30,7 +30,8 @@ module.exports = connection => {
const settings = {
client: 'pg',
useNullAsDefault: true,
connection
connection,
...knexSnakeCaseMappers()
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good if we could use the knexfile here, rather than duplicating settings in 2 places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate ticket? :D

Copy link
Contributor

Choose a reason for hiding this comment

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

nah, it would take longer to write the ticket than to just make the change. But I see what you're saying, stay in your lane 🚗

@wheelsandcogs wheelsandcogs changed the title *WIP* AS-695: convert database to snake case AS-695: convert database to snake case Oct 8, 2018
@wheelsandcogs wheelsandcogs merged commit 5f8af4f into master Oct 8, 2018
@wheelsandcogs wheelsandcogs deleted the feature/convert-to-snake-case branch October 8, 2018 11:54
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