Summary

Clean up, modernize, and document the the Perforce codebase.

Review Request #8944 — Created May 17, 2017 and submitted May 25, 2017, 6:25 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

a7e06fc...

Reviewers

Groups

reviewboard

People

Description

The Perforce support has been around for a very long time, and there
were some warts and unused code that never got cleaned up.

This change removes some methods and parameters that weren't needed
anymore, rearranges some functions, removes unnecessary code, converts a
static method into a normal method, and adds documentation throughout
the file. It sets the stage for future work in this file.

Testing Done

Unit tests and real-world functionality work.

Built the docs (temporarily adding this file to the docs build -- it's
not by default) and made sure the docs were correct.

Issues

Description	From	Last Updated
This isn't really correct. While there is a yield in here, that's used by @contextmanager. The contextmanager wrapper will return …	david	May 24, 2017, 3:45 p.m.
Should be returns here too.	david	May 24, 2017, 3:45 p.m.

flake8 passed.

JSHint passed.

Ship it!

reviewboard/scmtools/perforce.py (Diff revision 1)

The issue has been resolved. Show all issues

This isn't really correct. While there is a yield in here, that's used by @contextmanager. The contextmanager wrapper will return a context manager, which isn't really a type in and of itself, but I think could be documented as object.

chipx86 May 24, 2017, 12:44 p.m.

Yeah I wasn't sure how we really want to document these. I feel like maybe a better option is to invent a Context: section, but napolean won't know it.

chipx86 May 24, 2017, 2:16 p.m.

I have a change coming up that will let us define a Context: section, which is probably going to be more clear for anyone reading the docs, given the nature of the function.

reviewboard/scmtools/perforce.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be returns here too.
```

Change Summary:


Updated to use Context instead of Yields. This will be available as part of a set of extensions to Sphinx that are being worked on (but renders fine even without them).
Made connect and run_worker public.
Improved the docs a bit for connect and run_worker.

Commit:

d9ebffbc7508346a057f55436b58ca321e76bb07

a7e06fc2629dd2b4e897b58ca0a032945a614fbc

Diff:

Revision 2 (+474 -134)

Show changes

	reviewboard/scmtools/core.py
	reviewboard/scmtools/perforce.py
	reviewboard/scmtools/tests/test_perforce.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (014fb77)