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

Helper Methods #111

Closed
ankane opened this issue Jan 5, 2020 · 13 comments
Closed

Helper Methods #111

ankane opened this issue Jan 5, 2020 · 13 comments

Comments

@ankane
Copy link
Owner

ankane commented Jan 5, 2020

I think helper methods can be very powerful for Strong Migrations (shout-out to @fatkodima for lots of work on this and GitLab for popularizing the concept), but it's a fundamental change from the approach Strong Migrations has taken in the past, so I wanted to discuss it here and get feedback from the community.

Trade-offs

Currently, Strong Migrations is focused on keeping the migration experience consistent with vanilla Rails and educating users on what's not safe. The advantages are:

  1. Users don't need to learn new methods
  2. Users get to learn a bit about databases

Helper methods change this by introducing new methods that make it less obvious about what's going on behind-the-scenes. The advantages are:

  1. Simpler code - the new methods don't require users to jump through hoops to make migrations safe
  2. Less to think about - many users may not care about what's going on behind-the-scenes

Choice

The good news is we can give teams a choice.

StrongMigrations.enable_helpers

This will:

  1. Add the helper methods to migrations
  2. Change the messages users see about how to make migrations safe

If one is overwhelmingly preferred, we can consider removing the other in the future.

The drawbacks of a choice are:

  1. Increased maintenance
  2. Increased complexity / possible confusion with docs, issues, etc

Implementation

Two options that come to mind are:

Prefix or suffix methods

add_foreign_key_safely # safe method
add_foreign_key        # unsafe method with safety checks

Override existing methods (eliminates need for safety_assured)

add_foreign_key  # safe method
add_foreign_key! # original method, no safety checks

This doesn't require users to learn new methods, but it's more invasive, as it changes the behavior of existing Rails methods.

Would love to hear everyone's thoughts.

This was referenced Jan 5, 2020
@fatkodima
Copy link
Contributor

Personally, I'd prefer suffix methods as it is obvious what is the difference, while I will always forget what is the difference between bang/nobang methods. And this won't change behavior of existing Rails methods.

As for StrongMigrations.enable_helpers, I think it will be better to not have any options to enable/disable it and always have those methods available to use.

To solve mentioned trade-offs, maybe it will be better to expand a little existing warning messages, something like this:

Changing the type of an existing column requires the entire
table and indexes to be rewritten. A safer approach is to:
1. ...
2. ...
3. ...
...

You can achieve this using change_column_safely ...

So, having 2 logical blocks: 1) general theory and 2) some notes on how process can be simplified by available methods, if available.

@Skipants
Copy link

Any interest in doing it in a separate gem? I feel like that would keep the focus of this one small and clear. It also makes it easy to opt-in or out via Bundler rather than gem configuration.

@ankane
Copy link
Owner Author

ankane commented May 12, 2020

Hey @Skipants, that sounds like the best approach. The gem could include the helpers as well as override Strong Migration's error messages to use the helpers.

@fatkodima Is this something you're interested in creating and running?

@fatkodima
Copy link
Contributor

Hey. I'm more on the side of mine approach.
I'm not interested in that, at least right now.

@ankane
Copy link
Owner Author

ankane commented May 12, 2020

No worries. However, I think it's better as a separate gem at this point. Going to close out the existing PRs, but I appreciate the contributions.

@kkumler
Copy link

kkumler commented Jun 25, 2020

Has anyone started on this separate gem, @ankane ?

@ankane
Copy link
Owner Author

ankane commented Jun 25, 2020

Hey @kkumler, not that I'm aware of.

@pirj
Copy link

pirj commented Sep 30, 2021

@kkumler There's https://github.com/doctolib/safe-pg-migrations that provides some helper methods.

@kkumler
Copy link

kkumler commented Sep 30, 2021

@pirj Thanks for the consideration to mention me after so long.
That project looks interesting, though I think the safe_by_default methods added into strong_migrations may satisfy the majority of user needs, but it's still definitely worth a look!

@pirj
Copy link

pirj commented Oct 6, 2021

@mvz You requested a safe rename_table helper. Fresh from the oven.

@mvz
Copy link

mvz commented Oct 7, 2021

Thanks, @pirj!

@pirj
Copy link

pirj commented Oct 7, 2021

Beware, safe-pg-migrations does disable_ddl_transaction! for ALL migrations.

@fatkodima
Copy link
Contributor

@mvz @pirj @Skipants @kkumler I created a separate gem with helpers 🎉 Aka strong_migrations on steroids.

It has helpers for renaming tables/columns, changing columns types (including changing primary keys from integer to bigint), adding columns with default values, adding different constraints, and many more, and be able to do data migrations on very large tables in background. Please, check out! https://github.com/fatkodima/online_migrations

Just need to make a release.

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

No branches or pull requests

6 participants