Add type hints and modernize documentation for reviews/views/

Review Request #12685 — Created Oct. 17, 2022 and submitted

Information

Review Board
release-6.x

Reviewers

This change adds type hints to the reviews.views module, and does some
updates to documentation (filling out a few missing docstrings and
editing others).

Ran unit tests.

Summary ID
Add type hints and modernize documentation for reviews/views/
This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.
6958c015cfaa8bc9f4dfa27bca4032536e31df4c
Description From Last Updated

'typing.Dict' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

Blank line between these.

chipx86chipx86

Missing Type:

chipx86chipx86

SafeString should precede mark_safe here.

chipx86chipx86

Blank line here.

chipx86chipx86

These will need to specify the actual module path to link up right.

chipx86chipx86

The description should be separated from the summary.

chipx86chipx86

We should probably have a * in here somewhere to separate positional from keyword-only arguments, to keep this maintainable. There …

chipx86chipx86

Same note about the module path.

chipx86chipx86

This will also need to specify the module path. Wish we had some automatic review that could catch these. We'll …

chipx86chipx86

line too long (82 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

unexpected spaces around keyword / parameter equals Column: 31 Error code: E251

reviewbotreviewbot

Question: Since this is optional do we have to say datetime.datetime or None for the type?

maubinmaubin
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. Fantastic cleanup!

    A few small things stood out, but nothing major.

  2. Show all issues

    Blank line between these.

  3. Show all issues

    Missing Type:

  4. Show all issues

    SafeString should precede mark_safe here.

  5. reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line here.

  6. Show all issues

    These will need to specify the actual module path to link up right.

  7. reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
     
     
     
     
    Show all issues

    The description should be separated from the summary.

  8. reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    We should probably have a * in here somewhere to separate positional from keyword-only arguments, to keep this maintainable. There might be other similar methods that could benefit.

  9. Show all issues

    Same note about the module path.

  10. Show all issues

    This will also need to specify the module path.

    Wish we had some automatic review that could catch these. We'll have to someday get automatic doc building as part of the tests.

    Can you build docs and see if anything comes up for any of these?

    1. Unfortunately I don't see any warning output.

  11. 
      
david
Review request changed

Commits:

Summary ID
Add type hints and modernize documentation for reviews/views/
This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.
12b65c4843d5ffc111cb061efe7ad2e20758d750
Add type hints and modernize documentation for reviews/views/
This change adds type hints to the reviews.views module, and does some updates to documentation (filling out a few missing docstrings and editing others). Testing Done: Ran unit tests.
3db5468075a577ca44bbf7984cb911927054dfda

Diff:

Revision 3 (+1100 -332)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
maubin
  1. 
      
  2. reviewboard/reviews/views/mixins.py (Diff revision 4)
     
     
    Show all issues

    Question: Since this is optional do we have to say datetime.datetime or None for the type?

    1. We probably could, but I think we can probably look at that at a case-by-case basis. This is pretty obscure API that we're probably rarely going to touch ever, much less have a third party use it.

    2. We generally don't say or None in the type (though we could, but historically we haven't, and type annotations can now convey that a bit better). Instead, it's more helpful to describe the condition under which a None result might be returned instead.

  3. 
      
maubin
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (d39b58f)
Loading...