Do not use capturing groups when unnecessary in URLs

Review Request #1469 — Created Nov. 20, 2018 and updated

guest3011
Review Board
c342f6c...
guest375
Given a URL pattern of the form `((?P<capture>[a-z]+)/)?`, a call to
reverse with the kwargs `{'capture': 'foo'}` will result in a
`NoReverseMatch` exception. This occurs because the `capture` param gets
hidden behind a positional parameter and Django will expect the
positional parameter intead of the named parameter for the call to
reverse. We now no longer use capturing groups -- we explicltly mark the
outer group as non-capturing (e.g. `(?:(?P<capture>[a-z]+)/)?`, which
works as expected.

We previously were also using capturing groups in the form of
`?(P<capture>(a|b))`. Not only will a non-capturing group work here, the
group isn't needed at all: a expression of the form `(?P<capture>a|b)`
is sufficient. All instances of this have been fixed.

Testing Done:
- Ran unit tests.
- Viewed the diff fragment view for an interdiff.
- Viewed the e-mail preview views in html and text formats.

Reviewed at https://reviews.reviewboard.org/r/10273/


  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
guest3011
  1. 
      
  2. reviewboard/reviews/urls.py (Diff revision 1)
     
     
    There was no need to change this one; it's fine as is.
  3. 
      
guest375
  1. 
      
  2. reviewboard/accounts/urls.py (Diff revision 1)
     
     

    What about other formats?

    1. Other formats are fine. Rejected.
  3. 
      
Loading...