Docutils: Documentation Utilities / Patches / #192 Codebase modernisation

Günter Milde - 2022-01-26

Thank you for the patch set. I spent the day reviewing and implementing.
Not changed:
* some set/dict literals where the "tratidional" form seems more clear,
* error catching: here we should check for the most appropriate (sub)class to use,
* deprecated and generated modules,
* assignments before return, when they improve code readability.
* the recommonmark test (works here as-is, fails with proposed changes) What recommonmark version do you use for the test? Which OS?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Adam Turner - 2022-01-26

Thanks! I really appreciate it.

error catching: here we should check for the most appropriate (sub)class to use,

OK. Python 3 did change IO and Environment errors to be identical to OSError -- but as you say the exception hierarchy was reworked so e.g. FileNotFoundError or whatever may be more appropriate.

the recommonmark test

As currently it is just a no-op statement. (I don't have recommonmark set up for docutils on my laptop, which is probably why I missed that tests fail -- sorry!) Perhaps just remove?

some set/dict literals where the "tratidional" form seems more clear,

OK. For html4css1 Line 159, perhaps special_characters = _html_base.HTMLTranslator.special_characters.copy() if you don't like the {**...} form? It makes that the intention is to copy much clearer.

A

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Günter Milde - 2022-01-27
  
  On 2022-01-26, Adam Turner wrote:
  
  error catching: here we should check for the most appropriate
  (sub)class to use,
  
  Done in [r8989].
  
  (I don't have recommonmark set up for docutils on my laptop, which is
  probably why I missed that tests fail -- sorry!) Perhaps just remove?
  
  The recommonmark test are skipped unless a matching version is installed.
  They test the wrapper that normalises/fixes the doctree returned from
  recommonmark. IMO, we should keep the test as long as we are supporting
  "recommonmark" (which currently is the only compatible Markdown parser
  packed in Debian/stable).
  
  Related
  
  Commit: [r8989]
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
  - Adam Turner - 2022-01-27
    
    Perhaps just remove?
    
    Sorry, I meant remove the change I proposed!
    
    A
    
    If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Adam Turner - 2022-01-26

@milde I noticed you removed some extra u characters and some combining marks compared to my original patch.

Patch to fix (along with some other things): https://github.com/AA-Turner/docutils/pull/13 // https://github.com/AA-Turner/docutils/pull/13.patch

A

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Adam Turner - 2022-01-26
  
  (The some other things are some str.join generator expression changes, removing one more set of parens, and the copy change above. In the str.join part, there is a redundant code block removed -- nested_tags is never used)
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Günter Milde - 2022-01-27
  
  On 2022-01-26, Adam Turner wrote:
  
  I noticed you removed some extra u characters and some
  combining marks compared to my original patch.
  Patch to fix (along with some other things)
  
  Thank you for the fixup.
  
  The some other things are some str.join generator expression changes,
  
  Here, I did also some refactoring to avoid hard to read
  "parenthesis clusters" like
  
  ... + % (self.name, ' < '.join((path, *(pth for pth, opt + in reversed(include_log))))))
  
  removing one more set of parens
  
  In the context of keyword arguments, I prefer the parentheses over a
  space because of operator precedence
  cf https://www.python.org/dev/peps/pep-0008/#other-recommendations
  
  Thanks again.
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
  - Adam Turner - 2022-01-27
    
    avoid hard to read "parenthesis clusters"
    
    Thanks! I tried to be as minimal as possible in the patch, agree with this change though.
    
    A
    
    If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Günter Milde - 2022-05-03

Are there possible improvements left or can we close this ticket?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Adam Turner - 2022-05-03

There are always improvements! But this issue specifically about 2.7 cleanup I think can be closed.

A

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Günter Milde - 2022-05-10

status: open --> closed-fixed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Günter Milde - 2022-05-10

A large part of the proposed cleanups are now implemented.
Others are outdated after the drop of 2.7 support or not agreed on.
Thanks for contributing.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Codebase modernisation

Group

Searches

Help

#192 Codebase modernisation

Discussion

Related