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

Don't run rubocop on schema dumping #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidlibrera
Copy link

@davidlibrera davidlibrera commented Apr 9, 2018

I think that the gem should not run rubocop on my local schema.rb. In my specific case my .rubocop.yml has different configuration and it conflicts everytime I run rubocop.

I also changed the way tests run so is not necessary to run rubocop in order to let specs passing.

@Tolsto
Copy link

Tolsto commented Apr 11, 2018

Running Rubocop with autofix is a core part of how this Gem resolves the schema.rb conflicts. Not every project uses RuboCop. And not every project that does use RuboCop has a RuboCop configuration that'd be compatible with schema.rb. Removing Rubocop would render the Gem more or less useless since it wouldn't work anymore for projects with no Rubocop or an incompatible .rubocop.yml.
You should exclude schema.rb from your local .rubocop.yml configuration if the generated schema.rb is incompatible with it.

@davidlibrera
Copy link
Author

davidlibrera commented Apr 11, 2018

I don't understand your answer.
If a project uses rubocop each member of the project must run rubocop before commit anything. If a project doesn't use rubocop none of the project members have to run it. schema.rb is autogenerated and the only thing that can conflict is columns order. And this gem do it very well (https://github.com/jakeonrails/fix-db-schema-conflicts/blob/master/lib/fix_db_schema_conflicts/schema_dumper.rb)

If a project uses rucobop and for some reason some cops should run on schema.rb this gem creates another kind of conflicts (every time I run migration then I must run my own rubocop again).

@Tolsto
Copy link

Tolsto commented Apr 11, 2018

We experienced conflicts that went beyond the ordering. I assume that's why the Gem uses Rubocop with autofix.
Why do you have to run your own Rubocop after a migration? The schema will get dumped when you run a migration and the schema dumper will then invoke Rubocop.

@davidlibrera
Copy link
Author

davidlibrera commented Apr 11, 2018

For example our team decide to use single quote for simple strings. This gem uses the default rubocop configuration, so it uses double quote.
This way, running migrations will update schema.rb with double quote and then I must run again it in order to remove the double quoting.

It is very invasive!

@Tolsto
Copy link

Tolsto commented Apr 11, 2018

Why would you have to run Rubocop again? I also use Rubocop enforced single quotes for simple strings in all of my projects but I do not enforce my style guide for schema.rb as I don't see a reason for forcing auto-generated files to follow a specific style. For schema.rb it's only necessary because the generator does not produce consistent outputs.

@davidlibrera
Copy link
Author

davidlibrera commented Apr 11, 2018

Because I want it. If you don't like to have code style on schema.rb it doesn't means that other people can't do it.
This way the gem force me to do something in a certain way.

Maybe there are some situations that I discard because I never found it.
Can you show me an example of conflic between two teammate that occurs without running rubocop?

@alexanderadam
Copy link

What about a compromise and making rubocop optional instead removing it completely?

@davidlibrera
Copy link
Author

@alexanderadam much much better. But I would like to know why it should run rubocop in order to remove schema conflicts. In 100% of cases conflicts in my team are related to columns order, so after reordering column everything works fine.
The example inside the README is about column order, the only lib file
https://github.com/jakeonrails/fix-db-schema-conflicts/blob/master/lib/fix_db_schema_conflicts/schema_dumper.rb
sort column by name.

Looking the code the only reason to run rubocop are for let spec pass (inside a test there is a here document with a schema.rb example and only after rubocop execution the test pass).
If there are no other reason to run rubocop I think it is useless

https://github.com/jakeonrails/fix-db-schema-conflicts/blob/master/spec/integration/integration_spec.rb

@alexanderadam
Copy link

@davidlibrera Just to clarify this: I'm not related to this gem. I just stumbled about this issue here and thought maybe there's another solution besides all or nothing. 😉

@guigs
Copy link

guigs commented May 23, 2019

We experienced conflicts that went beyond the ordering

@Tolsto It would be useful to know which kind of conflicts besides ordering can happen in the schema. Do you have any example?

@jakeonrails
Copy link
Owner

Hi there, I can answer this.

I am unsure why this ever would happen - it seems like a bad decision for the Rails schema dumper, but back when this gem was originally written (for Rails 4.x) my team was seeing code like this generated by an unmodified schema dump:

# My schema.rb
t.string :name, null: false,                length: 20

# My teammates schema.rb
t.string :name, null: false,    length: 20

Depending on the ordering of the columns, the extra spacing would vary between null/length, creating conflicts. So Rubocop is used to normalize the horizontal whitespace in addition to ensuring everything is sorted alphabetically to create deterministic output.

It may be that later versions of Rails (5/6.x) no longer generate non-deterministic horizontal whitespace, in which case disabling the Rubocop pass is probably OK, but I don't know if this is the case off the top of my head based on my recent work.

@jakeonrails
Copy link
Owner

To add on further, at some point I remember this being suggested in another issue and my response then was to find a way to use the rubocop.yml of the host application instead of the one built into this library, if one exists in the host application. I think it would make sense for several reasons, including that it would allow this gem to continue working whenever Rubocop changes their cop names between version upgrades as a host application would only have to update their local rubocop.yml in order to fix any issues with this gem. I believe this solution would also fix your problem - you have a different way you want your code to be formatted to comply with your own Rubocop rules.

I would be happy to accept a PR making that change, but one disabling the Rubocop pass does not make sense to me.

@hugopeixoto
Copy link

It may be that later versions of Rails (5/6.x) no longer generate non-deterministic horizontal whitespace, in which case disabling the Rubocop pass is probably OK, but I don't know if this is the case off the top of my head based on my recent work.

This was changed in 5.1.0. Rails no longer adds horizontal whitespace. Here's the pull request: rails/rails#25675

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.

6 participants