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

Allow table name to be Symbol or String, remove run from macros #660

Merged

Conversation

matthewmcgarvey
Copy link
Member

I had heard that using run in macros slowed down compilation times. Turns out, it's true!

I ran benchmarks on master and this branch (hyperfine --warmup 3 'crystal build --release spec/model_spec.cr')
master: 5.016 seconds (average)
with these changes: 4.156 seconds (average)

That's a 17% improvement in release compile time!

Changes

Our only use for run was to infer the table name in migrations and in the models. We turned the model name into a symbol that was the table name. We only ever needed this info at runtime so it made sense to bring the code that was in the file being run. The only problem is that we can't return a symbol anymore because the table name isn't determined until runtime and it's not possible to construct symbols at runtime. I added a TableName alias that is just String | Symbol and updated everywhere that takes in a table name to use it. I also found that all the places we were using Model::TABLE_NAME could be replaced by the class method Model.table_name so I removed the constant as well.

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Yeah, I love this idea especially since speeding up the compile time can also help increase dev turn-around time.

I don't think we really lose any compile-time checks here because the schema enforcer should (in theory) be the one checking if you typed table "usres" instead of table "users", right?

@matthewmcgarvey
Copy link
Member Author

Yep, no loss of safety, schema enforcer was already converting the symbol to a string https://www.github.com/luckyframework/avram/tree/master/src/avram/schema_enforcer/validation.cr

@matthewmcgarvey matthewmcgarvey merged commit bc41988 into luckyframework:master Apr 3, 2021
@matthewmcgarvey matthewmcgarvey deleted the mm/infer-table-name branch April 3, 2021 18:08
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