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

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

Closed
greytape opened this issue Mar 15, 2023 · 6 comments
Closed
Labels
enhancement New feature or request

Comments

@greytape
Copy link

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
Copy link
Owner

djezzzl commented Mar 16, 2023

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 djezzzl added the enhancement New feature or request label Mar 16, 2023
@djezzzl
Copy link
Owner

djezzzl commented Mar 16, 2023

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

@greytape
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

djezzzl commented Apr 14, 2023

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
Copy link
Owner

djezzzl commented May 1, 2023

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!

@djezzzl djezzzl closed this as completed May 1, 2023
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

No branches or pull requests

2 participants