- Issue created by @zniki.ru
- Merge request !10679Resolve #3495968: Move entity_test_entity_types from entity_test.module β (Open) created by zniki.ru
- πΊπΈUnited States smustgrave
I think this one is well scoped. There are ton of modules in system but this particular one's helper module is used clearly in a number of spots.
Refactor looks good to me.
- π¨πSwitzerland berdir Switzerland
+1, this does add the same class as π Move helpers in entity_test.module and delete it Needs work , feels like this could have been a single issue as it's probably a lot of overlap of the classes that have to be adjusted between those two.
- π¨πSwitzerland berdir Switzerland
Unsure on whether or not this should provide BC like the other issue, but I haven't found a single usage of entity_test_entity_types() in contrib, so I think that's OK.
- π·πΊRussia zniki.ru
Thanks for review.
I decide to split issue to smaller scope to simplify review process.
If you think it will be better to combine this issue with parent issue, please let me know.
I can see many issues with giant MR. It's difficult even for me to do self review, when it comes to giant MR.
Please share some documentation or personal experience how to detect when issue new to be spitted to several smaller issues, or when it's better to keep all at the same place.
Thanks for feedback once again. - π¨πSwitzerland berdir Switzerland
How to best split larger changes into multiple issues is a hard problem to solve and an ongoing discussion. There are no strict rules and there's usually tradeoffs. In this case, it's smaller, easier to review issues versus conflicts and overlaps between multiple issues. I'm not sure myself it would be better to split the the two issues and keep them separate. I agree that all of entity_test.module was too much for a single issue and splitting it up makes sense, just not if 2 or 3 issues is the best solution but that's not so important, it's a minor difference.
An old but still valid resource or this is https://webchick.net/please-stop-eating-baby-kittens. But it doesn't offer a definitive answer on this.
- π³πΏNew Zealand quietone
My didn't find usages either, https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...
@nikolay shapovalov, there are the Issue scope guidelines for Drupal core issues β and in there is a section about size and LOC, Merge Request size β . These are guidelines to ensure quality, not a set firm rule. The key part is that the "ideal merge request size varies with the type of fix". And like you say it is also about simplifying the review process. The MR, what ever the size or LOC change must me manageable for a reviewer in a reasonable amount of time, the research suggests 1 hour max at a time.
- π·πΊRussia zniki.ru
Thank you @berdir, @quietone. For the links and the explanations.
Automatically closed - issue fixed for 2 weeks with no activity.