#341 Halp widget
Merged by abompard. Opened by abompard.
abompard/fedora-hubs halp_widget  into  develop

Download 341.patch

This is the branch implementing #98 and #325.

The Halp widget is more complex than most widgets : the UI is React-based because it is interactive, it has custom views and a cached function to get the help requests from Datagrepper, and it has a custom config panel (#325). I've made reusable React components where possible. I've also introduced i18n in the JS UI, it's better to do it earlier rather than later.

Maybe lowercase "help"?

I recommend adding a docblock here.

I recommend adding docblocks on this class and its methods below.

For PEP-8, flask, fedmsg, and requests should be imported in the second group with jinja below.

I recommend docblocks on this class and all of its methods below.

I recommend removing this commented code.

Docblock.

I recommend documenting the msg argument and its type here.

For PEP-8, the flask above should eb grouped with this flask and should have a space separating it from the builtins.

I recommend a docblock here.

I recommend docblocks here and on its functions.

I recommend a docblock for this class.

It would be good to document the args and kwargs and what they are used for.

docblock

It would be good to document the args and kwargs here too.

This method is a bit long and thus hard to follow. I recommend breaking it up into helper methods.

It would be good to document the args and kwargs.

This method could use a docblock.

This class and its methods could use docblocks.

It seems like there was a lot more Python code added than tests. Is there 100% coverage on the new code? If not, I recommend adding more tests.

All my comments are just suggestions and are at your option. LGTM.

1 new commit added

  • Implement most of the review comments

I've added most of the docblocks, except on view methods where there isn't more to describe than in the parent class. In that case I've just removed the docblock to make it clear.

I'm going to add more unit tests.

6 new commits added

  • Implement most of the review comments and add tests
  • Add tests for the halp widget
  • Map channels in messages to configured channels
  • Make widget views emit the template signals
  • Add the Halp configuration panel
  • Add Halp widget

@bowlofeggs : are you happy with the changes?

Very often circular imports indicate that two modules should be combined, or that something they both use should be a third module. I recommend considering what this circular import might be telling you about an organization issue.

@abompard Sure, it's looks good to me!

Pull-Request has been merged by abompard

Metadata