Replace bespoke plugin handling with stevedore #12
Conversation
|
Link to the rendered README https://github.com/eggmaster/bugjira/blob/field_handling_revision/README.md |
|
LGTM In general, some questions but non-blocking ones. |
|
|
||
|
|
||
| class FieldGenerator: | ||
|
|
There was a problem hiding this comment.
(non-blocking) suggestion: Other files classes docstrings were really helpful while reviewing this PR. Could you add to this class as well?
There was a problem hiding this comment.
Yes! thank you for noticing this.
|
|
||
|
|
||
| class FieldDataGeneratorFactory(): | ||
| """Factory class that returns the FieldDataGenerator associated with the |
There was a problem hiding this comment.
praise: In general, I really like the docstrings of this PR. Thanks for this.
| def test_get_field_class_bad_key(): | ||
| """ | ||
| GIVEN an instance of the FieldGenerator class | ||
| WHEN we call _get_field_class with an invalid generator_type |
There was a problem hiding this comment.
(non-clocking) question: Why this generator type is invalid? I've double checked with test_get_field_class generator_type BUGZILLA is the same as the one at L117
There was a problem hiding this comment.
BUGZILLA is only used to instantiate FieldGenerator here. "bad key" is the invalid generator_type parameter to the _get_field_class method. I'll add a comment to make this clearer in the test.
eggmaster
force-pushed
the
field_handling_revision
branch
from
993df8d
to
fdae382
Compare
There was a problem hiding this comment.
Wow. That was a lot :-). Couple comments, but overall this looks very good. Thorough documentation and tests, code seems solid, if a bit complex. One suggestion for future changes of this magnitude. If possible, maybe try to get to this point in stages. I think it will be easier for reviewers to understand the context of the various changes, and make it easier to see why a certain chunk of function is being refactored out. In this case, for example, I would probably suggest doing something like:
- start with a patch that only removed the custom plugin-loading code to the point that stevedore could be pulled in to replace some of that. This should show the new code does something similar to the old code, but with 'x' improvement (lower loc, fewer potential errors, whatever)
- extract out a top-level factory/interface. This shows how the new code will be called, and any changes that may be required around that change
- extract out the field-level factory stuff. Shows the lower level getting more flexible
- anything else that wasn't covered yet.
I suspect you went through similar steps on the way to this patch, and that all of this is crystal clear for you, since you have been working through it the whole time. And let me repeat, the code seems very solid! However, I think if you were to include the reviewers in the process by submitting more incremental changes, it would be much easier for them to understand what is happening with substantially less effort.
Instead of using a custom-made plugin loader, use the more powerful and flexible stevedore library. Also, utilize the factory pattern to handle field data generation. This refactor also addresses a shortcoming in the code it replaces wherein there was a hard- coded assumption that a single plugin would provide field configuration for BOTH jira and bugzilla. The new code supports separate plugins for each. Note that even though the FieldDataGeneratorFactory will create a separate plugin for each backend type, both plugins get their data from the same contrib/sample_fields.json file. This is unlikely to be the approach in any production use of bugjira; users will instead want to implement their own plugins and "advertise" them to bugjira using the stevedore entry points approach.
eggmaster
force-pushed
the
field_handling_revision
branch
from
fdae382
to
4178032
Compare
|
@jguiditta I meant to say thanks for your suggestions for splitting the patch. Thanks! |
There was a problem hiding this comment.
This is really well laid out, I love all the doc strings and unit tests. I would have liked to see a unit test with stevedore in action as well to understand the mechanism better, but I'm not familiar enough with stevedore to know if that would be too convoluted to test for. LGTM!
Instead of using a custom-made plugin loader, use the more powerful and flexible stevedore library.
Also, utilize the factory pattern to handle field data generation. This refactor also addresses a shortcoming in the code it replaces wherein there was a hard- coded assumption that a single plugin would provide field configuration for BOTH jira and bugzilla. The new code supports separate plugins for each.
Note that even though the FieldDataGeneratorFactory will create a separate plugin for each backend type, both plugins get their data from the same contrib/sample_fields.json file. This is unlikely to be the approach in any production use of bugjira; users will instead want to implement their own plugins and "advertise" them to bugjira using the stevedore entry points approach.