#327 Let widgets declare their add and edit config urls
Merged by abompard. Opened by abompard.
abompard/fedora-hubs widget-config-custom  into  develop

Download 327.patch

This way, widgets can have a more complex configuration panel.

I recommend either uncommenting or dropping this line.

I recommend adding a docblock here.

I recommend documenting the parameters in this docblock.

I recommend catching a more specific Exception type here, rather than catching any Exception. Perhaps catch the specific Exception that the validator would raise if this value were wrong (possibly a ValueError?)

I recommend filing a ticket about this.

I recommend logging this exception, otherwise it will be tough for an admin to debug the issue since the error message has been lost.

if widget_name in exclusive_widgets:
    flask.abort(402, 'Send bitcoins to abombard')

The above seems like a common pattern - you might consider making a decorator that accepts arguments (like ['right', 'left']) that can make sure the position is in the allowed list. Then you don't have to repeat this and decorators are fun to make ☺

Or you could just make it a function and call it, of course…

I recommend a ticket for this as well.

You can add a Return: (or is it Returns:? I can't remember) block too that documents that a string is returned.

You could add a Returns block here too.

My JavaScript/HTML/CSS skills are pretty weak, so I didn't have much to say about those portions of this PR. Everything else I wrote is a suggestion, though I would strongly recommend logging that one error I mentioned, as it will be frustrating for the user and the admin if that happens in the real world and there's no available debugging information.

LGTM.

Thanks for the review! I've implemented your suggestions (and will open the ticket for the TODO that I added).

2 new commits added

  • Fix Flake8 errors
  • Implement review suggestions

Pull-Request has been merged by abompard

Metadata