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

Add lucky db.migrations.status CLI task #135

Merged

Conversation

DevinRiley
Copy link
Contributor

@DevinRiley DevinRiley commented Jul 4, 2019

👋

This is my first attempt at a contribution to Lucky and pretty much my first time writing Crystal code, so any / all feedback is super appreciated.

I took at a stab at adding the lucky db.migrations.status task from #126

A few things to note:

Example output:
Screen Shot 2019-07-06 at 6 41 37 AM

Thanks!

Devin Riley added 2 commits July 4, 2019 10:53
- Adds a new CLI task lucky db.migrations.status
- Exposes a formerly private method on Avram::Migrator::Runner to create the migrations table
  if it doesn't exist
@DevinRiley DevinRiley force-pushed the add-db-migrate-status-task branch from b3e308a to b8351ca Compare July 4, 2019 19:33
Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Thanks so much for your contribution! It looks fantastic. Just a few minor suggestions before we merge this in

Could you run this task somewhere here in script/test

$COMPOSE lucky db.migrate.one

That works like our "integration test" to make sure things don't fail in unexpected ways.

@@ -137,6 +137,12 @@ class Avram::Migrator::Runner
end
end

def setup_migration_tracking_tables
Copy link
Member

Choose a reason for hiding this comment

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

Could you make this a class method instead? I think that might make more sense since it doesn't need an instance variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in f5dd3b1

Note that I also pulled up create_table_for_tracking_migrations into a private class method, since it too does not depend on instance variables but is called by setup_migration_tracking_tables

src/avram/tasks/db/migrations_status.cr Show resolved Hide resolved
private def migration_statuses
migrations.map do |migration|
status = if migration.new.migrated?
"Migrated".colorize(:green)
Copy link
Member

Choose a reason for hiding this comment

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

I think the yellow and green clash a bit. Could you leave this the default color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 69e3111

Now looks like this:
Screen Shot 2019-07-06 at 6 41 37 AM

@DevinRiley
Copy link
Contributor Author

@paulcsmith This is ready for another look

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

@paulcsmith paulcsmith merged commit 00d97ba into luckyframework:master Jul 6, 2019
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