-
Notifications
You must be signed in to change notification settings - Fork 619
Speed up status command and sort output #997
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
base: main
Are you sure you want to change the base?
Conversation
|
Should I try to address those linter failures by replacing the old-style |
|
I tried running the lint check on my own machine and got some unfortunate looking errors. 🤞 |
552e75a to
d295509
Compare
|
Rebased to |
|
Anything else I can help with or provide? |
Nope, I've been going back and forth on the behavior. Specifically, whether it's a good idea to modify the default sort order. There's a bit of a decision to make because if the issues below ever get implemented, "grouped migrations" might have the same timestamp. (In Postgres, we have Granted, your PR handles this case, because it checks the timestamp and if the same, sorts by version. I think for now, we should keep the existing behaviour and default listing by version number. We can still roll forward with the List query, just modify the sort. Wdyt? The idea is that in the future, we can add a The latter is probably useful for those running out-of-order migrations, and they can choose which sorting method they prefer. By timestamp order (this PR d295509, pending always goes last) After |
|
TL;DR - let's create 2 functions sortByTimestamp (your current PR) and sortByVersion (current behavior), and keep the current behavior the default. But use the List query to speed things up, WDYT? In the future, we can add a |
|
sounds good to me! I thought about sort orders but I thought I would keep it simple for the first pass. |
bdd1367 to
b31e7b8
Compare
|
|
Just be frank if you hate the name of the sort order flag, that's easy to change ;-) |
|
... and if you'd rather have the new sorting flag split out into a separate PR, I can do that, too. |
|
Sorry for all the back forth. Crazy time end of year wrapping up projects. If you could split the cli stuff that'd be great. The main changes here that are nice to get in are the improved list stuff |
b31e7b8 to
c4bdc72
Compare
c4bdc72 to
7ae06a5
Compare
👍👍 I just took the last commit off and also rebased to the latest changes in There's no flag to change sorting order and no function implementing the alternate sorting order. |
|
erk, I needed to push 1 more commit to get rid of that sort-order function. |
Fixes #282
Commit 1 retrieves migration application timestamps in
ListMigrations(), which requires updating every db-specific querier;Commit 2 uses the updated
ListMigrations()to retrieve status of all migrations in a single db query;Commit 3 sorts the output of
goose statusso