Organization of views package is poor

[seanh]

Problem

The organization of the lms/views/ package isn't ideal. Here's the current layout:

lms/views/
    __init__.py
    index.py
    application_instances.py
    authentication.py
    basic_lti_launch.py
    config.py
    content_item_selection.py
    error.py
    favicon.py
    feature_flags_test.py
    reports.py
    status.py
    api/
        __init__.py
        error.py
        lti.py
        canvas/
            __init__.py
            authorize.py
            files.py
    helpers/
        __init__.py
        _canvas_files.py
        _via.py
        frontend_app.py
    predicates/
        __init__.py
        _helpers.py
        _lti_launch.py

There are a number of problems with this:

1. We don't have a good place to put "close collaborators" for the views.

   Things that are not themselves views but are "helpers" (e.g. helper functions, helper classes) for the views to call. Sometimes we do want to split code into a separate unit either because it can be reused by multiple different views, or just for the sake of splitting things up into smaller pieces. But that code doesn't belong in any of the other top-level directories (it's not a service, nor a validation schema, etc).

   We don't want to litter our view functions and view classes with _private_helper() methods and indented blocks of logic. We want to split things into separately unit-tested components.

   But those components do belong in the lms/views/ package.

   Currently the only place we have for such components is "helpers". For example lms/views/helpers/ and lms/views/predicates/_helpers.py. But it isn't nice to have to put everything that isn't a view in a helpers/ sub-package or helpers.py module.

2. We don't have a good place to put exception views.

   We can't put these in views/exceptions.py because exceptions.py is the name we use, throughout all packages and subpackages in our app, for modules that contain custom exception classes.

   Currently our exception views live in views/error.py as a result. But this is a bad name. Pyramid calls them exception views not error views. And not all exceptions are errors, not all exception views are error views.

3. It's hard to tell what contains views and what doesn't.

   Most views/foo.py files and views/foo/ packages contain views. But some don't. For example views/decorators/ contains view decorators and no views. views/predicates/ contains custom predicates and no views. There's nothing in the naming to distinguish whether a given "foo" module or package contains views or not.

4. It's kind of ugly to prefix everything with _'s and while this is best practice to help denote public API vs private internals, that doesn't really apply to the views package which doesn't really have a public API. It just contains views that Pyramid calls, and other things that are only called by those views. No other package in our code imports things from the views package. But _'s are being used in lms/views/ to denote the public API vs private internals of sub-packages like helpers/ and predicates/.

Solution

We want to reorganise the views following this pattern:

• Actual views functions and view classes go in views.py or *_views.py files within lms/views/
• Exception views go in exception_views.py files
• Things that are not views but are close collaborators of the views (helpers of different kinds, predicates, etc) go in foo.py files within lms/views/. They don't have to be prefixed with helpers or predicates. Rather, they lack the views suffix.
• A given foo.py can be imported and used by anything at the same level or below it in the hierarchy. For example lms/views/bar/foo.py can be imported by lms/views/bar/gar.py or by lms/views/bar/zar/*.py, etc.

Here's a rough guess at how the new layout might look:

lms/views/
    __init__.py
    ui/
        __init__.py
        index_views.py
        application_instances_views.py
        authentication_views.py
        config_views.py
        content_item_selection_views.py
        exception_views.py
        favicon_views.py
        feature_flags_test_views.py
        reports_views.py
        status_views.py
        lti_launch/
            basic/
                __init__.py
                predicates.py <-- Custom predicates used by views in both
                                  configured/ and unconfigured/
                configured/
                    __init__.py
                    views.py
                    configure_grading.py <-- The configure_grading() helper.
                unconfigured/
                    __init__.py
                    views.py
    api/
        __init__.py
        exception_views.py
        lti_views.py
        canvas/
            __init__.py
            authorize_views.py
            files/
                views.py
                helpers.py <- The canvas_files_available() and via_url() helpers.

Advantages of this:

1. We have a much nicer way of putting in close collaborators without them having to go in specific modules or subpackages like helpers or predicates.

2. Exception views can now go in correctly named exception_views.py modules or packages that don't clash with exceptions.py.

3. It's easy to tell what files contain views: they all end with views.py

4. We've gotten rid of all the unnecessary leading _'s

5. We've also organised all the non-API views into views/ui/.

[lyzadanger]

What think you of using a view_ prefix instead of suffix? I find the directory a heckuva lot easier to scan that way.

[lyzadanger]

        lti_launch/
            basic/
                __init__.py
                predicates.py <-- Custom predicates used by views in both
                                  configured/ and unconfigured/
                configured/
                    __init__.py
                    views.py
                    configure_grading.py <-- The configure_grading() helper.
                unconfigured/
                    __init__.py
                    views.py

At first blush this seems fairly...rigorous But I do see something I like here, which is the pattern:

a_logical_set_of_views/
   predicates.py
   some_helper.py
   views.py

With the idea that predicates, helpers, etc. that are relevant to a broader set of views would live higher up in the tree. And there is a single, obviously-named views module.

That would obviate the need to have foo_view.py naming conventions at the top level, but also involves some nesting, so, shrug?

[seanh]

What think you of using a view_ prefix instead of suffix? I find the directory a heckuva lot easier to scan that way.

I can see why it would be easier to scan, but I think suffix has a few advantages:

• foo_views.py matches the English language word order, "foo views"
• exception_views.py matches the phrase "exception views" from the Pyramid docs (and Pyramid methods like add_exception_view() and @exception_view_config)
• foo_views.py is in the same order as foo/views.py when it's in a subpackage. For example a grep or file open search for "foo view" will find both: the typical search rule is that it must contain all the letters and in the same order (but not contiguous)
• Although it doesn't scan as well it reads better to me. exception_views.py in particular reads better than views_exception.py.

[seanh]

``` lti_launch/ basic/ __init__.py predicates.py <-- Custom predicates used by views in both configured/ and unconfigured/ configured/ __init__.py views.py configure_grading.py <-- The configure_grading() helper. unconfigured/ __init__.py views.py ``` At first blush this seems fairly...rigorous

This could probably be simplified to:

        lti_launch/
            basic/
                __init__.py
                configured_views.py
                predicates.py 
                unconfigured_views.py

Assuming there aren't any collaborators that're specific to configured views and not unconfigured ones, or vice-versa, and you want to denote that via locality. Like configure_grading.py. It may not be necessary to show, via the file layout, that configure_grading.py is in fact only used for unconfigured views. Or, whatever functions are in configure_grading.py could just go directly in the configured_views.py module, if that wouldn't make the module too long.

The configured vs unconfigured thing is a separate issue. Currently they're all in BasicLTILaunchviews. But we noticed that they might split nicely into separate configured and unconfigured view classes.

[seanh]

But I do see something I like here, which is the pattern: ``` a_logical_set_of_views/ predicates.py some_helper.py views.py ``` With the idea that `predicates`, `helpers`, etc. that are relevant to a broader set of views would live higher up in the tree. And there is a single, obviously-named `views` module. That would obviate the need to have `foo_view.py` naming conventions at the top level, but also involves some nesting, so, shrug?

I think we'll end up with both the foo_views.py and the foo/views.py mixed together. If foo has any collaborators, and we want to make it clear that those collaborators are local to foo only, and we don't want to just put those collaborators in the foo_views.py file itself (e.g. that would make the file too long), then you need a foo/ dir. But if foo is just some views without any private collaborators, or at least without any that're truly local to foo and we don't think will be useful to other views one day, then it can just be a foo_views.py

[seanh]

In-module private collaborators

Not mentioned at the top of the PR but I think it'll also be fine to just put some or all of a view's close collaborators in the view modules themselves, when that makes sense. Example:

# foo_views.py

from pyramid.view import view_config

@view_config(...)
def foo(request):
    # Uses FooHelper.
    ...

class FooHelper:
    ...

# foo_views_test.py

from lms.views.foo_views import foo, FooHelper

class TestFoo:
    # Unit tests for foo(). These might patch FooHelper.
    ...

class TestFooHelper:
    # Direct unit tests for FooHelper.
    ...

In this case I don't think we need to strictly require that TestFoo should patch FooHelper like we normally would do, either.

Reasons not to do this:

• If FooHelper is called by anything outside of foo_views.py
• If it would make either foo_views.py or foo_views_test.py too long (lots of small collaborating files, rather than a few big ones, please)

I do think we should lean quite heavily towards splitting things out into separate files. Lots of small, collaborating, potentially reusable things. As a general rule I think tending to split things proactively up is going to lead to better code, compared to refusing to split things up until you see a good reason to. And I still have visions of Atomic Jolt's views/foo.py files containing a single foo() view and then 1000 lines of deeply entangled random private helper functions with no unit tests and that eventually turned out to be reducable to 50 lines of code. Nonetheless, sometimes an in-module private collaborator is going to make sense and it should be allowed.

[seanh]

Plural file names

I think we should go for foo_views.py and foo/views.py rather than foo_view.py, because:

• I want to be allowed to put more than one view in a single module
• I think opening foo_view.py (singular) and finding multiple views is surprising
• I think opening foo_views.py and finding only one view is less surprising. The file contains all the foo views. It just so happens that there's only one foo view right now. But if there are more foo views in future, they'll go in foo_views.py
• I want it to be consistent, not mix *view.py and *views.py
• It's convenient if a search for *views.py finds all view modules, rather than having to use the more complicated search term *view[s].py
• Avoid having to do *view.py → *views.py renames (or vice-versa)
• Avoid the inevitable view.py that contains multiple views and views.py that only contains one, if we allow both styles

[seanh]

I'd like to suggest an alternative to this that I think might be nicer:

• Instead of appending _views (or prepending views_) to all of the files that do contain views, let's prepend _ to all the helper files that don't contain views.

• So the foo views would just be lms/views/foo.py

• But the bar helpers would be lms/views/_bar.py

Because:

• The views_*.py or *_views.py approach keeps repeating "views" over and over again, twice in every path. lms/views/foo_views.py etc. Almost every file in lms/views/ ends up ending in _views.py.

• Merely having to prepend an _ to the non views modules is less bad both because _ is shorter than _views and because there are fewer non-view modules than view modules in the views package.

• A leading _ is the standard way to denote "internal use" in Python:

  Even with __all__ set appropriately, internal interfaces (packages, modules, classes, functions, attributes or other names) should still be prefixed with a single leading underscore.

  ... and this fits: the views themselves are for external use (they're for Pyramid to call) but view helpers are only meant to be used internally within the lms/views/ package.

• The pattern should apply just as well to other packages besides views. I think we should probably apply it to all packages throughout our apps. For example a util package with a public foo module and an internal bar module:

  lms/util/ init.py foo.py _bar.py

• It's less renaming to get all of our apps from where they are now to this new pattern. Don't need to append _views.py to every single existing views module.

One tricky detail is about Pyramid exception views. In other packages we use exceptions.py as the module name for the package's custom exception classes. In views packages I think we should use exceptions.py to contain both any custom exception classes and any exception views. And most likely there won't be any custom exceptions in views packages anyway since views are sort of the "top" of the app (but there are in fact some in h). But anyway exceptions.py modules can just contain both custom exception classes and exception views.

lms/views/ might end up looking something like this:

lms/views/
    __init__.py
    ui/
        __init__.py
        index.py
        application_instances.py
        authentication.py
        exceptions.py
        ...
        _some_helper.py
        _another_helper.py
        lti_launch/
            __init__.py
            configured.py
            unconfigured.py
            _configure_grading.py
            _predicates.py
    api/
        __init__.py
        exceptions.py
        lti.py
        canvas/
            __init__.py
            authorize.py
            files.py
            _canvas_files_available.py
            _via_url.py

[seanh]

^ The above suggestion also avoids any "view" vs "views" singular vs plural confusion. If a file contains only one view should it be foo_view.py or should it be foo_views.py like the other view files? If it's foo_view.py and then I add a second view to it do I have to rename the file? I predict it will be impossible to get people to stick consistently to any answer to this question so you'll inevitably end up with view.py and views.py files and some of the view.py ones will contain multiple views. Better to avoid the whole thing

[seanh]

Closing this as it's moved here: https://github.com/hypothesis/dev/pull/1 (this new layout hasn't actually been applied to lms's views yet but I don't think we need to keep this issue open)