fedora-hubs

#323 Redesign the widget system to be class-based
Merged by abompard. Opened by abompard.
abompard/fedora-hubs widget-class  into  develop

Download 323.patch

OK, this is a big one. This branch redesigns the widget plugin system in hubs to use classes instead of Python modules.

In this new world, widgets are Python classes in their own module, which is usually in their own directory (but they don't have to be).

Here are the components involved for a widget:

  • a widget class
  • one or more view(s) that will be registered with the app
  • templates which are now in a subdirectory of the widget itself
  • cached functions, which are functions whoose results get cached and invalidated when the right message is recieved on the bus (as before). There can be as many cached functions as necessary per widget.

I would recommend reading the integrated documentation, you can build it with the tox -e docs command and read it with xdg-open ./docs/_build/html/api.html. If there's something unclear, please tell me, I'll fix it.

Improvements over the current system :

  • widgets can reside anywhere that is Python-accessible, which means they don't have to be inside the hubs app itself : widgets can be added from external modules, which makes it easier to have out-of-tree widgets, for people wanting to contribute to hubs or having their own site-specific widgets (there may be other installs of hubs one day).
  • widgets can have more than one view, and the root view (ex-data function) isn't any different from the others
  • the files are in their own directory, including templates (but not CSS files or JS, that's for later) which makes it easier to know what belongs to a widget and what belongs to the hubs app
  • the widget registration isn't static, it can be set in the config file, which means that the admin can disable or enable more widgets without touching the code
  • there's no implicit decoration of the functions anymore, which made it hard to understand where the data was coming from
  • the caching is separated from the view context, which enables view to have no caching, or to use multiple cached function to generate their output (with different invalidation policies)
  • there's way less global variables (like the widget templating environment, that one widget could edit and involuntarily propagate the change to other widgets).
  • the chrome template decoration system has been replaced with jinja template extention, it's the exact use cas for this feature.
  • the widget parameters have gained a label key which will make configuration more human-readable
  • probably other minor things I forgot... ;-)

I recognize that it's a big change, so I'd like to add the following :

  • try to read commits in order, I've made them topic-specific, so you can understand what's being changed and why. Reading the whole "Files changed" page is actually much harder.
  • I'm available for any questions you may have
  • I've converted all existing widgets, and if you don't want to convert the widget you're working on, I pledge to convert it myself (it's actually not that hard once you understand how the pieces fit together)

I'm working my way along the commits and I'll leave comments inline as I run across stuff. I'll let you know when I've made it all the way through my review (and I don't know that I'll finish today).

It might be better to use the scheme python.path.to.module:TheModuleAttribute since that's what is used in most other Python tooling I've seen (pdb when specifying breakpoints, Python entry points, etc).

Not a big deal, but when I see all dots I think that the last thing is also a module.

Did you switch this from a namedtuple to a class so that it could be extended? If so, you can easily extend namedtuples by taking the old class's _fields list and creating a new one like this.

s/instanciate/instantiate

Is this left-over from something?

I'm guessing this is a way to let the function just define a set of topics/categories it wants to trigger a refresh? Makes sense. My preference is to not have commented-out blocks of code and to make issues/RFEs instead, though.

This seems like a reasonable thing to log at debug level so I think it'd be good to uncomment it.

Can you not just pass self.execute rather than using lambda?

It's a little touch, but I like to link this sort of stuff out to documentation:

`Jinja2 <http://jinja.pocoo.org/docs/>`_

should work. So should

`Jinja2`
.. _Jinja2:
   http://jinja.pocoo.org/docs/

It's a total nitpick and you should feel free to not bother :smile:

Ugh, I can't edit that comment, but I noticed right after I submitted the second example should be 

`Jinja2`_
.. _Jinja2:
   http://jinja.pocoo.org/docs/

\o/

I think you want Attributes: here since these are class attributes.

It might be better to use the scheme python.path.to.module:TheModuleAttribute

Yeah I hesitated. Django doesn't use the colon, neither does nose2, so it's not unanimous. I'll just switch to it if it's more familiar for you guys, I just think we should pick one and stick to it :-)

Did you switch this from a namedtuple to a class so that it could be extended?

Actually, the tuple behavior of Argument wasn't used anywhere in the code, so it felt like this should have been a class all along. It's also more extendable, we can provide defaults for some attributes if necessary (e.g the label could be derived from the name if not given).

I think you want Attributes: here since these are class attributes.

Fixed, thanks.

Can you not just pass self.execute rather than using lambda?

Right, that's a leftover from when this method had an argument.

I'm guessing this is a way to let the function just define a set of topics/categories it wants to trigger a refresh? Makes sense. My preference is to not have commented-out blocks of code and to make issues/RFEs instead, though.

I agree, I'll remove it.

Other fixes done, thanks a lot for your review.

1 new commit added

  • Implement fixes from jcline's review

Isn't this an abstract method?

Isn't this an abstract method?

Yeah, now that I think of it it doesn't make much sense to have a default implementation that will invalidate the cache on every message. I'll make it abstract.

1 new commit added

  • Make CachedFunction.should_invalidate() abstract

The universe started only after this PR :alien:

Edited by kushal

typo? :)

The universe started only after this PR 👽

agreed!

[@pingou] typo?

Nope, that's actually how Tox recommends doing it: http://tox.readthedocs.io/en/latest/example/basic.html#depending-on-requirements-txt

2 new commits added

  • Sort the widget registry by name
  • Fix the help text in the widgets config template

45 new commits added

  • Sort the widget registry by name
  • Fix the help text in the widgets config template
  • Make CachedFunction.should_invalidate() abstract
  • Implement fixes from jcline's review
  • Allow the widget to declare where the views and cached functions are
  • More explicit widget validation errors
  • Make a real class for WidgetParameter
  • Improve API doc crossreferences
  • Build the API documentation with Sphinx
  • Adapt dev-guide documentation
  • Fix compatibility with Python 2
  • Move widget templates inside the widget directory
  • Replace the AGPLv3 template variables with template tags
  • Replace the chrome wrapper with template inheritance
  • Convert the workflow widgets
  • Convert the subscriptions widget
  • Convert the sticky widget
  • Convert the stats widget
  • Convert the rules widget
  • Convert the pagure_pr widget
  • Convert the pagureissues widget
  • Convert the memberships widget
  • Convert the meetings widget
  • Convert the linechart widget
  • Convert the library widget
  • Convert the github_pr widget
  • Convert the githubissues widget
  • Convert the fhosted widget
  • Convert the feed widget
  • Convert the fedmsgstats widget
  • Convert the dummy widget
  • Convert the contact widget
  • Convert the bugzilla widget
  • Convert the badges widget
  • Convert the about widget
  • Adapt the cache scripts to the new CachedFunction
  • Adapt the backend workers to the new CachedFunction
  • Add the Flask variables and filters to widget template env
  • Adapt views and templates to the new widget class
  • Rename get_widget to get_widget_instance
  • Adapt the models to the new widget class
  • Create a widget registry class
  • Create a base class for widgets
  • Add a class for widget-specific views
  • Add a CachedFunction class

This does not look right.

This does not look right.

The registry is a subclass of OrderedDict, so it's basically a dict with additional features.
self[key] = value is how one would set a key to the dict from an internal method. What bothers you with it?

This does not look right.

The registry is a subclass of OrderedDict, so it's basically a dict with additional features.
self[key] = value is how one would set a key to the dict from an internal method. What bothers you with it?

Sorry, My mistake

I changed it to an OrderedDict so the admin could choose in which order the widgets appear on the "Add new widget" config box. But if you prefer the old way, I can revert this commit and just sort the list alphabetically in the view.

I skimmed the PR again, and I'm generally very happy with it. Fully disclosure, I didn't spend tons of time scrutinizing the widget conversions, but I think we can handle any bugs in that as they come up.

Thanks for doing this, it looks great :clap:! I'm satisfied, but I'll defer to sayan since this is big enough to deserve a couple pairs of eyes.

Thanks for your review Jeremy.

This PR looks great to me too. :ok_hand: I am setting up the hubs-devel.fic.org and planning to deploy this.

Great, I'm merging it then. Thanks a lot for you reviews! :)

My offer to migrate the widgets you're working on still stands if you need it :)

Pull-Request has been merged by abompard
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.