- Download Diff

Summary:

Docs for URL,Dashboard,NavigationBar-Hooks

Review Request #3617 — Created Dec. 3, 2012 and submitted Dec. 4, 2012, 10:13 p.m.

Information
Owner:	slchen
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

Extensions documentation for URLHook, DashboardHook, 
and NavigationBarHook.

Testing Done

Ran `make html` in reviewboard/docs/codebase/

No error output from sphinx.

Rendered HTML pass visual inspection.

Issues

Description	From	Last Updated
These are only really useful if we're actively linking to them.	chipx86	Dec. 4, 2012, 5:08 p.m.
"Review Board"	chipx86	Dec. 4, 2012, 5:11 p.m.
Hooks aren't really there to help "customize" Extensions - they're there to give extensions a means to customize Review Board. …	mike_conley	Dec. 4, 2012, 5:50 p.m.
"Review Board"	chipx86	Dec. 4, 2012, 5:11 p.m.
"We briefly examine a few...demonstrage usage with sample code" - on tone, sounds a bit text-bookish. Maybe instead, simply: "The …	mike_conley	Dec. 4, 2012, 5:58 p.m.
We can make this even more general - "URLHooks are used to extend the URL patterns that Review Board will …	mike_conley	Dec. 4, 2012, 6:01 p.m.
This last sentence is generally true for all hooks - maybe you won't need it if you add that sentence …	mike_conley	Dec. 4, 2012, 6:02 p.m.
"once is" -> "once"	chipx86	Dec. 4, 2012, 5:11 p.m.
SampleExtension ? Probably don't need to capitalize "Dashboard." Is this example meant to tie in with the DashboardHook? If so, …	chipx86	Dec. 4, 2012, 5:14 p.m.
Alignment error.	chipx86	Dec. 4, 2012, 5:16 p.m.
"dashboard page" ?	chipx86	Dec. 4, 2012, 5:16 p.m.
"on the dashboard's navigation pane."	chipx86	Dec. 4, 2012, 5:17 p.m.
"dashboard page."	chipx86	Dec. 4, 2012, 5:17 p.m.
"extension" "dashboard page"	chipx86	Dec. 4, 2012, 5:18 p.m.
I think it's better to make this one statement.	chipx86	Dec. 4, 2012, 5:31 p.m.
Blank line above.	chipx86	Dec. 4, 2012, 5:32 p.m.
Make sure the indentation inside the template tags are correct: {% block content %} {% box "reports" %}	chipx86	Dec. 4, 2012, 5:34 p.m.
Instead of "To do..." how about something like, "This is my new Dashboard page for Review Board!" That way, it …	mike_conley	Dec. 4, 2012, 6:03 p.m.
Probably want a blank line between these.	chipx86	Dec. 4, 2012, 5:35 p.m.
End with a period.	chipx86	Dec. 4, 2012, 5:35 p.m.
It'd be nice to just pass the entries in here directly.	chipx86	Dec. 4, 2012, 5:44 p.m.
They don't use them as a primary mechanism, they are a primary mechanism. I'd be OK with "Extensions can use …	mike_conley	Dec. 4, 2012, 7:18 p.m.
Let's say this: Notice how sample_extension.urls was included in the patterns. In this case, sample_extension is the package name for …	mike_conley	Dec. 4, 2012, 7:21 p.m.
"it then" -> "then it"	mike_conley	Dec. 4, 2012, 7:21 p.m.
This looks like leftover cruft to be removed.	mike_conley	Dec. 4, 2012, 7:22 p.m.

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

These are only really useful if we're actively linking to them.

slchen Dec. 4, 2012, 5:09 p.m.

Roger; removed the ones not being referenced by a link somewhere.

docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
"Review Board"
```
docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
"Review Board"
```
docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
"once is" -> "once"
```

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

SampleExtension ?

Probably don't need to capitalize "Dashboard." Is this example meant to tie in with the DashboardHook? If so, maybe make that clear and link to the section on that.

"a URL"

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

Alignment error.

slchen Dec. 4, 2012, 6:44 a.m.

Should it be:

urlpatterns = patterns('sample_extension.views',
    url(r'^$', 'dashboard'),
)

?

I grepped reviewboard repo for "urlpatterns" and they all seem to follow that structure, but have more entries for "url"

docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
"dashboard page" ?
```
docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
"on the dashboard's navigation pane."
```
docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
"dashboard page."
```
docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
"extension"

"dashboard page"
```

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

I think it's better to make this one statement.

docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
Blank line above.
```

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

Make sure the indentation inside the template tags are correct:

{% block content %}
{%  box "reports" %}

docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
Probably want a blank line between these.
```
docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
End with a period.
```
docs/codebase/extending/extensions.txt (Diff revision 1)
Show all issues
```
It'd be nice to just pass the entries in here directly.
```

Great to see some documentation! Just a few suggestions below.

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

Hooks aren't really there to help "customize" Extensions - they're there to give extensions a means to customize Review Board. They're one of the primary mechanisms for changing the appearance and behaviour of Review Board.

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

"We briefly examine a few...demonstrage usage with sample code" - on tone, sounds a bit text-bookish.

Maybe instead, simply:

"The following hooks are available for extensions to use."

You might want to mention that hooks need only be instantiated for Review Board to "notice" them, and that these hooks are automatically removed when the extension shuts down.

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

We can make this even more general - "URLHooks are used to extend the URL patterns that Review Board will recognize and respond to."

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

This last sentence is generally true for all hooks - maybe you won't need it if you add that sentence in the parent section - that hooks need only be instantiated to work, and will be automatically removed on shut down.

docs/codebase/extending/extensions.txt (Diff revision 1)

Show all issues

Instead of "To do..." how about something like,

"This is my new Dashboard page for Review Board!"

That way, it makes it more clear what we're showing, and looks less like we forgot to put something in the documentation.

Change Summary:

Fixed various issues as per Christian and Mike's comments.

Diff:

Revision 2 (+148 -1)

Show changes

docs/codebase/extending/extensions.txt

docs/codebase/extending/extensions.txt (Diff revision 2)

Show all issues

They don't use them as a primary mechanism, they *are* a primary mechanism.

I'd be OK with "Extensions can use "hooks" as a mechanism for customizing Review Board's appearance and behavior."

Also, make sure to use "Review Board". Not ReviewBoard or Reviewboard.

docs/codebase/extending/extensions.txt (Diff revision 2)

Show all issues

Let's say this:

Notice how sample_extension.urls was included in the patterns. In this case, sample_extension is the package name for the extension, and urls is the module that contains the patterns.

docs/codebase/extending/extensions.txt (Diff revision 2)
Show all issues
```
"it then" -> "then it"
```
docs/codebase/extending/extensions.txt (Diff revision 2)
Show all issues
```
This looks like leftover cruft to be removed.
```
docs/codebase/extending/extensions.txt (Diff revision 2)
```
Good fix!
```

Change Summary:

Fixed issues as per Mike's latest comments

Diff:

Revision 3 (+145 -1)

Show changes

docs/codebase/extending/extensions.txt

Ship it!

```
Looks good to me. Thanks Sampson!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-1.7.x (d076073). Thanks!

You have a pending review.

Review Board 5.0.2 alpha 0 (dev)

Docs for URL,Dashboard,NavigationBar-Hooks

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: