s/dictionnaries/dictionaries/

@abompard can you rebase?

i am sorry but, @abompard wouldn't it be a little better if we had the doc strings for the functions? Please don't hate me for this. (you can consider me crazy, its fine since i like harley quinn)

I think these are an SQLAlchemy database session and an instance of hubs.models.Widget respectively, is that correct? It would be good if the documentation be explicit about it? I'm never sure when I see the word session if it means database session, request session, etc.

I moved the widget documentation to be a module-level docblock in https://pagure.io/fedora-hubs/pull-request/302#_5,23 and it's inserted into the Sphinx project and linked to in the dev-guide. Assuming people like that, after this gets merged I'll just rebase and move this block in with the rest of the documentation. I just wanted to let you know so you're not surprised if this moved in the near future :)

It would be good to add a docblock here so it shows up in auto-generated API documentation. I've been using the so-called Google style Sphinx supports since I find it the most readable, but we should probably just agree on one of the thee styles Sphinx supports (the others are plain RST and NumPy style) and be consistent. I'm happy to change my existing PRs to match.

Same note about adding a docblock as above

Having this execute at the bottom of this module makes me nervous. The problem I see in the future is some of the routes in this file get factored out (into one or more API modules, for example) and then import order can break whether or not the url rules are registered.

Long term I think the best thing to do is to make this app.py strictly about configuring the Flask app object and handle route definitions in a different module or modules. We should then use the package's __init__.py to strictly control import order and this initialization to ensure people can't import things in different orders and have weird things happening.

In the short term it might be best to move this to the __init__.py after a set of import statements that ensures all widget modules have been imported and initialized. Thinking about it, this might include importing the widget package. Does that make sense?

I've added the docstrings (except for one method which is actually internal-only, so I've prefixed it with an underscore like the convention recommends).

Having this execute at the bottom of this module makes me nervous.

I agree that we may be having a problem with the import order in the future. But I would argue that in that regard our problem comes from the way we register widgets: they are all imported in hubs/widgets/__init__.py, and that module is imported at the top of hubs/app.py.

I would totally agree to split views in a separate file, and then we could have a more "sane" way of registering widget views (importing views first, then widgets, then widget views).

I would also add that it kinda is "the Flask way" to register routes as the views are imported, since most of the time it's done via decorators, or module-level function calls (http://flask.pocoo.org/docs/0.12/views/), so the way I'm proposing here isn't fundamentally different, at least not from an import-order point of view.

But I totally agree it's messy. In fact, I'd be very happy to suggest a PR to change the way widgets are registered, which would be more easily customizable by the admin and wouldn't have to happen so early in the init process. But I'm afraid it would change quite a few things for people who are currently working on a widget.

Do you think it's worth doing anyway?

In fact, I'd be very happy to suggest a PR to change the way widgets are registered, which would be more easily customizable by the admin and wouldn't have to happen so early in the init process. But I'm afraid it would change quite a few things for people who are currently working on a widget.

Do you think it's worth doing anyway?

I certainly wouldn't object to a PR that improves the widget registration process. Although it might disrupt current on-going widget work a little bit, in my experience adjusting designs earlier rather than later causes less overall pain.

Also, I think this is okay to merge this PR as-is, but it is a "gotcha" we'll hit down the road when we decide to refactor. I've noticed a lot of the flask projects we have are precariously close to tons of circular imports because views are implemented in the same module the flask application object is created and configured. Views make use of functionality in other modules, and those modules in turn want to use Flask configuration values so they need to import the app object. Taking advantage of the init.py to configure the package and ensure a certain import order and that everything with views that require registration are, in fact, imported should solve that problem.

All that being said, it sounds like we're in agreement and I don't think that's a problem we need to solve in this PR. How about I make an issue where we can flesh out the details? This PR looks good to me so merge away!

Yep, looks like we agree :-)