search

brendon / ranked-model

An acts_as_sortable/acts_as_list replacement built for Rails 4+
https://github.com/mixonic/ranked-model
MIT License
1.09k stars 134 forks source link

Remove NonNilColumnDefault exception from ranked-model.rb #181

Open vanboom opened 2 years ago

vanboom commented 2 years ago

For your consideration...

Raising the NonNilColumnDefault exception causes the Rails app to not even boot. Allow users to react to this requirement without crashing their application after adding the gem.

brendon commented 2 years ago

Hi @vanboom, what does .fatal do in this situation. Does it prevent the app from running until the problem is fixed. If it just logs out and carries on then I'm not a huge fan. This is something that needs to be fixed right from the start.

Perhaps we could improve the error raised?

vanboom commented 2 years ago

The logger message simply prints out the requirement but does not cause an exception. Thanks for considering this change. The reason we suggested it was that we added ranked-model to our Gemfile, then the app crashed with the exception because our rank column had a default value. We are adding ranked-model to an existing model with code in place to manage the position value. The exception causes an issue with deployment to production.

The NonNilColumnDefault exception requires that the database migration be released to production (to correct the default value requirement) prior to the gem being added to the application (because adding the gem renders rake impossible). This would cause the production application to break in the interim.

So our suggestion would be to print a warning similar to the way gems print deprecation warnings without raising a formal exception. That would allow developers to recognize the warning message, develop on a branch that includes the migration, then make one merge to main of 100% working code vs. releasing breaking code to production. Thanks for your consideration - this is a brilliant gem!

brendon commented 2 years ago

Thanks @vanboom. Good points. Let's continue the discussion on the issue thread for now until we come to an agreement :)

brendon commented 3 months ago

Closing this since we couldn't come to an agreement. I don't think it's too unreasonable to expect the database conditions to be correct when the ranking is declared on boot. Perhaps the workaround would have been to create a new column for the ranking and then rebuild the list based on the old gem's ordering, then remove that column. There were just a lot of issues where users were setting a default on the column, so this avoided all that pain for me.

vanboom commented 3 months ago

Thanks for reviewing this. Imho, having the exception block the rake migration step which would allow the dev to correct the database condition is too aggressive. The warning method would allow devs to correct the migration without the extra steps of removing and re-adding the gem. Rake should be allowed to run. Thanks again for your consideration.

brendon commented 3 months ago

Fair enough. We just need to figure out how to detect the CLI environment vs a running app. I definitely don't want the app to boot if there's a default column value.

brendon commented 3 months ago

Turns out the check is in a middleware: ActiveRecord::Migration::CheckPending. Not sure if it's gross to add a middleware for our purposes?