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

"Migrations are pending" popup is too aggressive #540

Closed
swanson opened this issue Dec 5, 2024 · 13 comments · Fixed by #570
Closed

"Migrations are pending" popup is too aggressive #540

swanson opened this issue Dec 5, 2024 · 13 comments · Fixed by #570
Assignees
Labels
enhancement New feature or request

Comments

@swanson
Copy link

swanson commented Dec 5, 2024

I often am running bin/rails g migration SomeMigration to get a blank migration and then writing it my editor instead of the generator. When I do this, the "Migrations are pending" popup immediately displays and asks me to run the (empty) migration.

From my perspective:

  • I would like this popup to ignore empty migrations that I just created
  • I would like the ability to disable this particular popup -- I wasn't able to find out how to do this in the list of "features enabled" in the main LSP

I think this has the potential for people to run the empty migration without realizing it ("I just click that button always") and then write the actual migration code and be confused why the changes don't seem to work (migration was marked as run so it doesnt re-run)

@andyw8 andyw8 added the enhancement New feature or request label Dec 5, 2024
@andyw8
Copy link
Contributor

andyw8 commented Dec 5, 2024

Hi Matt, thank you for the suggestion. We will consider some options for this.

@andyw8
Copy link
Contributor

andyw8 commented Dec 16, 2024

@swanson We discussed as a team and are considering this approach:

We won't prompt to run the migrations if the most recent migration is empty.

A migration would be considered empty if it has no methods:

class CreateFoo < ActiveRecord::Migration[8.0]
end

or if it contains only a timestamp column (which the generator may add by default).

class CreateFoo < ActiveRecord::Migration[8.0]
  def change
    create_table :foos do |t|
      t.timestamps
    end
  end
end

If were were do this, I don't think we wouldn't need an option to disable the popup.

The only downside I can think of is if you save the migration mid-way through writing it, then it would trigger the popup.

Any thoughts?

@swanson
Copy link
Author

swanson commented Dec 16, 2024

If I run bin/rails g migration ThisIsEmpty

the output is

class ThisIsEmpty < ActiveRecord::Migration[8.0]
  def change
  end
end

so I don't think that would solve my particular issue because the LSP would not consider that empty, correct?

@swanson
Copy link
Author

swanson commented Dec 16, 2024

Maybe "smart detection of empty migrations" is not the right solution, but it does seem like people (myself included) will want a way to turn off this popup, as it is pretty invasive if you aren't using it. The Rails "run pending migration" button is also an option so I don't believe this functionality is novel enough to be "always on".

@andyw8
Copy link
Contributor

andyw8 commented Dec 16, 2024

so I don't think that would solve my particular issue because the LSP would not consider that empty, correct?

We can have it treat that as empty too.

@andyw8
Copy link
Contributor

andyw8 commented Jan 8, 2025

@swanson we're considering this as an alternative.

@swanson
Copy link
Author

swanson commented Jan 10, 2025

@swanson we're considering this as an alternative.

Thanks but I'd really just like a way to disable this specific feature at this point!

@jackc
Copy link

jackc commented Jan 31, 2025

This is also an annoyance for me. #553 is an improvement I suppose, but it still seems like a half measure. What if the popup also included a button to open/edit the pending migration files?

That would actually be useful immediately after running rails generate migration, as it currently takes several clicks to open the newly generated file.

@vinistock
Copy link
Member

@swanson do you think that would be helpful? I would also prefer that over checking timestamps.

@swanson
Copy link
Author

swanson commented Jan 31, 2025

I use https://marketplace.visualstudio.com/items?itemName=tmikoss.rails-latest-migration so it wouldn't be something I would use. I can currently (without the LSP at all), run bin/rails g migration and then use this addon to quickly jump to the generated migration.

Personally, I still think an option to completely disable this would be my preference. I've not used it once, I use the built-in Rails "run pending migrations" button if there are new migrations from a merge that I am missing. And every time I generate a migration, it is popping up and I have to dismiss.

@laughingman-hass
Copy link

laughingman-hass commented Feb 2, 2025

Hi, I appreciate the intension with the migration pending popup, but as @swanson mentioned it's really aggressive to the point that it's getting in the way of development. Also adding that rails in development mode errors and reminds you that there are pending migration, so it would be ideal to have this configurable to be disabled.

@laughingman-hass
Copy link

As an additional thought, having to an option to dismiss and don't be reminded of migrations for the latest pending migration would be a decent step in the right direction. However this should be in addition to being able to disable the reminder all together.

@vinistock
Copy link
Member

We discussed among the team and decided to allow disabling the dialog #570. It's easier than trying to detect what the user intention or preference is.

having to an option to dismiss and don't be reminded of migrations for the latest pending migration would be a decent step in the right direction

The server can't modify the user's settings, so that would require the Ruby LSP to start allowing settings to be stored in some file database. That would also create an extra level of setting hierarchy, since editors like VS Code allow for user and workspace settings, but then we would additionally have the Ruby LSP's own setting store, which starts getting overly complicated.

If the LSP specification were to formalize a way for servers to suggest or prompt users to modify their settings, that would lead to a better experience and significantly less complicated implementation.

vinistock added a commit that referenced this issue Feb 3, 2025
Closes #540

Allow disabling the pending migration prompt through an addon setting. To disable, users can add this to their configuration:

```json
"rubyLsp.addonSettings": {
   "Ruby LSP Rails": {
     "enablePendingMigrationsPrompt": false
   }
 }
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants