Summary

Make ExtensionHooks easier to implement and manage.

Review Request #7680 — Created Oct. 7, 2015 and submitted Dec. 30, 2015, 2:45 a.m.

Information

Owner

chipx86

Repository

Djblets

Branch

release-0.10.x

Bugs

3731

Depends On

Reviewers

Groups

djblets

People

Description

ExtensionHooks have always been pretty simple. Constructing them will
initialize them, shutting down will permanently destroy them, and
subclasses only needed to override __init__() and shutdown(). This was
almost too simple.

Once destroyed, a hook had to be re-constructed in order to be
re-enabled, invalidating any signal connections that may exist or other
registered data. Extensions that needed finer-grained state management
had to do it entirely within the hook through its own custom
enabled/disabled states, which still left them initialized and
registered on the extension.

This change improves this in a couple key ways. First, initialization
can now live in initialize(), which does not need to call the parent
method (unless inheriting from another ExtensionHook that defines
initialize()). The shutdown code in shutdown() no longer needs to call
the parent method either.

ExtensionHooks can be disabled by calling disable_hook(), and then
re-enabled (with new state) by calling enable_hook(). The instance will
remain around, but the hook's registration and state will be properly
handled. The hook can also be instantiated with start_enabled=False to
create the hook instance without enabling it, allowing the extension to
do that later in its process.

Along with all this, I've fleshed out the docs quite a bit.

Testing Done

Djblets and Review Board unit tests pass.

Issues

Description	From	Last Updated
This patch is marked for release-0.10.x.	brennie	Nov. 3, 2015, 5:09 p.m.
No comma after otherwise.	brennie	Nov. 3, 2015, 5:09 p.m.
0.10?	brennie	Nov. 3, 2015, 5:09 p.m.
You don't need pass with a docstring.	brennie	Nov. 3, 2015, 5:10 p.m.
0.10?	brennie	Nov. 3, 2015, 5:10 p.m.
0.10	brennie	Nov. 3, 2015, 5:10 p.m.
"Enables" is the wrong tense.	AD adriano	Dec. 3, 2015, 12:02 a.m.
"re-constructing" -> "reconstructing".	AD adriano	Dec. 3, 2015, 12:02 a.m.
This is not a list.	brennie	Dec. 16, 2015, 3:29 p.m.
This is not a list.	brennie	Dec. 4, 2015, 12:44 a.m.
Not a list.	brennie	Dec. 16, 2015, 3:29 p.m.
Not a list.	brennie	Dec. 4, 2015, 12:44 a.m.
Does this render correctly? If not, we should put 'This attribute is optional.' in the description instead of here.	brennie	Dec. 16, 2015, 3:29 p.m.
Same here.	brennie	Dec. 16, 2015, 3:30 p.m.
Here, too.	brennie	Dec. 16, 2015, 3:30 p.m.
And here.	brennie	Dec. 16, 2015, 3:30 p.m.
And here.	brennie	Dec. 16, 2015, 3:30 p.m.

Tool: Pyflakes
Processed Files:
    docs/djblets/conf.py
    djblets/extensions/extension.py
    djblets/extensions/hooks.py
    djblets/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    docs/djblets/conf.py
    djblets/extensions/extension.py
    djblets/extensions/hooks.py
    djblets/extensions/tests.py

djblets/extensions/hooks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This patch is marked for release-0.10.x.
```
djblets/extensions/hooks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No comma after otherwise.
```
djblets/extensions/hooks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
0.10?
```

djblets/extensions/hooks.py (Diff revision 1)

The issue has been dropped. Show all issues

You don't need pass with a docstring.

chipx86 Nov. 3, 2015, 5:10 p.m.

We don't need it, but it communicates intent, that "we know there's no code here, just docs." Same reason I always put a break; after the last case in a switch.

djblets/extensions/hooks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
0.10?
```
djblets/extensions/hooks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
0.10
```

Change Summary:

Fixed the documentation to say 0.10, and not 0.9.

Diff:

Revision 2 (+366 -70)

Show changes

	djblets/extensions/extension.py
	djblets/extensions/hooks.py
	djblets/extensions/tests.py
	docs/djblets/conf.py

Tool: Pyflakes
Processed Files:
    docs/djblets/conf.py
    djblets/extensions/extension.py
    djblets/extensions/hooks.py
    djblets/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    docs/djblets/conf.py
    djblets/extensions/extension.py
    djblets/extensions/hooks.py
    djblets/extensions/tests.py

Ship it!

```
Ship It!
```

djblets/extensions/hooks.py (Diff revision 2)

An opportunity to use (bool, optional).

djblets/extensions/hooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"Enables" is the wrong tense.
```
djblets/extensions/hooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"re-constructing" -> "reconstructing".
```

djblets/extensions/hooks.py (Diff revision 2)

Another opportunity for (bool, optional).

djblets/extensions/hooks.py (Diff revision 2)

Another opportunity for (list, optional).

djblets/extensions/hooks.py (Diff revision 2)

Another opportunity for (object or class, optional).

djblets/extensions/hooks.py (Diff revision 2)

Another opportunity for (bool, optional).

djblets/extensions/hooks.py (Diff revision 2)

Another opportunity for (unicode, optional).

djblets/extensions/hooks.py (Diff revision 2)

Another opportunity for (list, optional).

```
I think this will close issue 3731.
```

Change Summary:

Updated some of the docs to call out optional parameters and fix typos.

Bugs:

3731

Diff:

Revision 3 (+366 -70)

Show changes

	djblets/extensions/extension.py
	djblets/extensions/hooks.py
	djblets/extensions/tests.py
	docs/djblets/conf.py

Tool: Pyflakes
Processed Files:
    docs/djblets/conf.py
    djblets/extensions/extension.py
    djblets/extensions/hooks.py
    djblets/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    docs/djblets/conf.py
    djblets/extensions/extension.py
    djblets/extensions/hooks.py
    djblets/extensions/tests.py

Ship it!

```
Ship It!
```

djblets/extensions/hooks.py (Diff revision 3)

The issue has been dropped. Show all issues

This is not a list.

chipx86 Dec. 4, 2015, 12:42 a.m.

Not a list in the data type sense, but a list in the "list of arguments" sense. The fact that these are tuple are internal details, but any list coming in as an argument in the form of *mylist becomes a tuple anyway. Officially, this is called a "variable length argument list," so I think the usage is okay here.

djblets/extensions/hooks.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This is not a list.
```
djblets/extensions/hooks.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Not a list.
```
djblets/extensions/hooks.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Not a list.
```

djblets/extensions/hooks.py (Diff revision 3)

The issue has been dropped. Show all issues

Does this render correctly? If not, we should put 'This attribute is optional.' in the description instead of here.

chipx86 Dec. 4, 2015, 12:42 a.m.

It does not, but many things we want to do don't. I'm planning to do some work to replace napolean with something that offers more choices like this. (This is also what others out there seem to be using for optional arguments, and it's handy information to have.)

djblets/extensions/hooks.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Same here.
```
djblets/extensions/hooks.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Here, too.
```
djblets/extensions/hooks.py (Diff revision 3)
The issue has been dropped. Show all issues
```
And here.
```
djblets/extensions/hooks.py (Diff revision 3)
The issue has been dropped. Show all issues
```
And here.
```

Change Summary:

Fixed some docs.

Diff:

Revision 4 (+366 -70)

Show changes

	djblets/extensions/extension.py
	djblets/extensions/hooks.py
	djblets/extensions/tests.py
	docs/djblets/conf.py

Tool: Pyflakes
Processed Files:
    docs/djblets/conf.py
    djblets/extensions/extension.py
    djblets/extensions/hooks.py
    djblets/extensions/tests.py



Tool: PEP8 Style Checker
Processed Files:
    docs/djblets/conf.py
    djblets/extensions/extension.py
    djblets/extensions/hooks.py
    djblets/extensions/tests.py

Status:: Completed
Change Summary:: Pushed to release-0.10.x (7648acc)