Add type hints and clean up some code for Mercurial.

Review Request #13609 — Created March 4, 2024 and submitted

Information

RBTools
release-5.x

Reviewers

This change modernizes the Mercurial backend somewhat through type hints
and general code housekeeping.

Ran unit tests.

Summary ID
Add type hints and clean up some code for Mercurial.
This change modernizes the Mercurial backend somewhat through type hints and general code housekeeping. Testing Done: Ran unit tests.
7da3ab1460ff3750d608449f3eedf79e83934a6d
Description From Last Updated

Can we define the type (for this and any others) in an "Instance variables" on the class body?

chipx86chipx86

I made a similar comment in another change, but I'd like to keep the original form, for the same reason …

chipx86chipx86

This should be in the multi-line form.

chipx86chipx86

We're getting a generator at the end, not a tuple. Probably isn't any better than a list (maybe worse). Though, …

chipx86chipx86

please, do not remove that parameter. I added this in /r/11102

miserymisery

Nit: These are all missing trailing periods.

maubinmaubin

These comments are missing trailing periods.

chipx86chipx86

Too many blank lines in the comment.

chipx86chipx86

Double spaces should be a single space after the period.

chipx86chipx86

The single backticks must be double backticks for ReST. Also the period should be after the parens.

chipx86chipx86

We should transition to keyword-only arguments here, given the mix of kwargs for the parent and an argument here.

chipx86chipx86

"Mercurial"

chipx86chipx86

The type is already defined on the class body.

chipx86chipx86

We have a more explicit type we can list here now.

chipx86chipx86

Same here.

chipx86chipx86
chipx86
  1. 
      
  2. rbtools/clients/mercurial.py (Diff revision 1)
     
     

    Can we define the type (for this and any others) in an "Instance variables" on the class body?

  3. rbtools/clients/mercurial.py (Diff revision 1)
     
     
     
     
     

    I made a similar comment in another change, but I'd like to keep the original form, for the same reason that we don't put } or ] on the final line for dicts/lists. It makes it easier to modify/add lines on multi-line strings without affecting previous lines.

    Also has a nice symmetry.

    Doesn't necessarily matter in this particular case, but I'd prefer it as a general guideline.

  4. rbtools/clients/mercurial.py (Diff revision 1)
     
     

    This should be in the multi-line form.

  5. rbtools/clients/mercurial.py (Diff revision 1)
     
     

    We're getting a generator at the end, not a tuple. Probably isn't any better than a list (maybe worse).

    Though, we're making an assumption here that we probably should check/handle with an exception handler. If we get too few or too many items here, we're going to crash. It's not a new problem, but worth considering.

  6. 
      
david
misery
  1. 
      
  2. rbtools/clients/mercurial.py (Diff revision 2)
     
     

    please, do not remove that parameter. I added this in /r/11102

  3. 
      
david
maubin
  1. 
      
  2. rbtools/clients/mercurial.py (Diff revision 3)
     
     
     
     
     
     
     
     

    Nit: These are all missing trailing periods.

  3. 
      
chipx86
  1. 
      
  2. rbtools/clients/mercurial.py (Diff revision 3)
     
     
     
     
     
     
     
     

    These comments are missing trailing periods.

  3. rbtools/clients/mercurial.py (Diff revision 3)
     
     
     
     
     

    Too many blank lines in the comment.

  4. rbtools/clients/mercurial.py (Diff revision 3)
     
     

    Double spaces should be a single space after the period.

  5. rbtools/clients/mercurial.py (Diff revision 3)
     
     

    The single backticks must be double backticks for ReST.

    Also the period should be after the parens.

  6. rbtools/clients/mercurial.py (Diff revision 3)
     
     
     
     
     
     

    We should transition to keyword-only arguments here, given the mix of kwargs for the parent and an argument here.

  7. rbtools/clients/mercurial.py (Diff revision 3)
     
     

    "Mercurial"

  8. rbtools/clients/mercurial.py (Diff revision 3)
     
     

    The type is already defined on the class body.

  9. rbtools/clients/mercurial.py (Diff revision 3)
     
     

    We have a more explicit type we can list here now.

  10. rbtools/clients/mercurial.py (Diff revision 3)
     
     

    Same here.

  11. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-5.x (d95f463)
Loading...