search

djezzzl / database_consistency

The tool to avoid various issues due to inconsistencies and inefficiencies between a database schema and application models.
MIT License
1.05k stars 46 forks source link

Ensure that generated todo files are created with consistent ordering #184

Closed greytape closed 1 year ago

greytape commented 1 year ago

Not sure if this is a bug or feature request, but we've noticed that regenerating the todo.yml after adding or removing a violation often (always?) results in some reordering of the checks. This makes it hard for PR reviewers (or anyone looking back over commit history) to see what has actually changed.

Would it be possible to change this so that the ordering of checks in todo files is consistent? This would mean that looking at the diff in the database_consistency.todo.yml, only the added or deleted violations would be visible.

I have copied out the suggested workflow from our internal docs below, to give you an idea of what we're doing that's giving rise to this behaviour. These are the steps that devs are supposed to follow when fixing or adding a violation. (NB there are some violations that we do not intend to ever fix, so we store these in a separate .database_consistency.config.yml).

## What if I have fixed an existing violation?

- Once you have fixed an existing violation you will need to regenerate the `.database_consistency.todo.yml`.
- You can do this by deleting the existing `.database_consistency.todo.yml`, then by running `database_consistency` with the `--generate-todo` flag:

    bundle exec database_consistency --config .database_consistency.config.yml --generate-todo

Happy to try and recreate a minimal reproducible example if this inconsistent ordering behaviour is surprising.

Thanks again for your work on the gem, which is really appreciated.

djezzzl commented 1 year ago

Hi @greytape,

Thank you for your kind words! I would be happy to help with this request!

The ordering of executing checks is always the same. We don't have any "randomizer" in between to shuffle it. Could you please provide or at least describe in differences only the steps?

djezzzl commented 1 year ago

P.S. I'm so happy you use the gem and have internal guidelines!

greytape commented 1 year ago

Ah, makes sense. I didn't think that a random reordering would be the default behaviour. 🙂 Perhaps this is happening because we are passing in another config file in alongside the generate-todo command? I will try and do some investigation this week at our end to see if I can give more guidance about why this might be occurring.

greytape commented 1 year ago

I've done a little bit of digging into this at our end, and it does seem like we've got a slightly niche case. We're seeing big differences in the diff partly as a result of moving files around the application (eg from app/models to app/domain_x). If and when we stop doing that our problems will be solved!

At present though, seems like the order is determined by the order of classes returned by ActiveRecord::Base.descendants. Does that seem right? This in turn is determined by when different clasess are loaded by rails.

I would have though it would be better to sort the classes alphabetically before writing the reports? That will ensure that the reports are always consistent when models are moved around the file structure of the app. I would also think that it would make the reports easier to read, having the top level objects organised alphabetically. What do you think?

Happy to open a PR if helpful.

djezzzl commented 1 year ago

Hi @greytape!

Thank you for the investigation and for sharing the results! I also think your suggestion makes a lot of sense.

Sorry that it took me long to respond! Could you please open the PR? I would be happy to merge it and release it.

djezzzl commented 1 year ago

I released it in 1.7.8. Please try it and let me know if it works for you. Thank you for the suggestion! Please keep it going!