Skip to content

Conversation

@tnwei
Copy link
Contributor

@tnwei tnwei commented Oct 23, 2020

As per #270 , submitting this pull request to revamp string formatting in pdoc to use f-strings. Also bumped min requirements of Python 3.5+ to Python 3.6+ in setup.py.

Following CONTRIBUTING.md:

  • All tests passed from python setup.py test
  • Ran flake8 --max-line-length 100, no new warnings. At time of writing, existing warning is:
 ./pdoc/test/example_pkg/_resolve_typing_forwardrefs/postponed.py:1:1: F407 future feature annotations is not defined

Copy link
Member

@kernc kernc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, pdoc/html_helpers.py and pdoc/templates/{html,pdf}.mako contain some further str.format() / str.__mod__ calls.

@tnwei
Copy link
Contributor Author

tnwei commented Oct 27, 2020

Note, pdoc/html_helpers.py and pdoc/templates/{html,pdf}.mako contain some further str.format() / str.mod calls.

You are right, will be checking on the missed files

@tnwei
Copy link
Contributor Author

tnwei commented Oct 28, 2020

Re-checked all documents to pick up on missed __mod__ and .format() calls:

  • pdoc/__init__.py
  • pdoc/cli.py
  • pdoc/test/__init__.py
  • pdoc/templates/html.mako

Edited:

  • pdoc/html_helpers.py (line 176)
  • pdoc/templates/pdf.mako (line 18)

@tnwei
Copy link
Contributor Author

tnwei commented Oct 28, 2020

Two questions:

pdoc/html_helpers.py (line 176)

str.format takes output from a map nested in list comprehension as below:

pdoc/pdoc/html_helpers.py

Lines 176 to 177 in 414739b

return '\n\n'.join('`{}`\n: {}'.format(*map(str.strip, line.split(':', 1)))
for line in filter(None, spec_with_desc.split('\n')))

Reformatting it would give

return '\n\n'.join(f'`{line.split(":", 1)[0].strip()}`\n:   '
            f'{line.split(":", 1)[1].strip()}' for line in filter(None, spec_with_desc.split('\n')))

but it looks rather inelegant. Are we good with this?

pdoc/templates/pdf.mako (line 18)

text = re.sub(r'^(?P<indent>( *))!!! \w+ \"([^\"]*)\"(.*(?:\n(?P=indent) +.*)*)',
lambda m: '{}**{}:** {}'.format(m.group(2), m.group(3),
re.sub('\n {,4}', '\n', m.group(4))),
text, flags=re.MULTILINE)

One of the inputs to str.format above include evaluating regex substitution where newline chars are involved. f-strings don't allow backslash in the curly braces, and assigning it to a variable will need rewriting the lambda function that is passed to re.sub(), like so:

def paragraph_fmt(m):
    sub = re.sub('\n {,4}', '\n', m.group(4))
    return f'{m.group(2)}**{m.group(3)}:** {sub}'
text = re.sub(r'^(?P<indent>( *))!!! \w+ \"([^\"]*)\"(.*(?:\n(?P=indent) +.*)*)',
                     paragraph_fmt, text, flags=re.MULTILINE)

Would love to have your comments @kernc

@kernc
Copy link
Member

kernc commented Oct 28, 2020

Snippets like these show str.format/% still has some value. And how awful regexes are.

I don't mind new variables in both cases. In the first case maybe:

spec, desc = map(...)
return f"..."

When in doubt, opt for clarity.

@tnwei
Copy link
Contributor Author

tnwei commented Oct 30, 2020

Snippets like these show str.format/% still has some value. And how awful regexes are.

Thanks for the thoughts, at some point I was contemplating if it's better to leave them as-is actually.

Anywho, I've finished updating each file in their own commit, whole PR is ready for review.

tnwei and others added 2 commits November 1, 2020 17:56
Reworded variable name to be more meaningful

Co-authored-by: kernc <kerncece@gmail.com>
@kernc
Copy link
Member

kernc commented Nov 27, 2020

Many thanks @tnwei! Hope you got your t(r)ee. 😄

@kernc kernc merged commit 78c06c6 into pdoc3:master Nov 27, 2020
kernc pushed a commit to johann-petrak/pdoc that referenced this pull request Jun 22, 2024
* MNT: Bumped Python version reqs to 3.6+

* ENH: Updated string formatting to use f-strings

* ENH: Resolved individual code comments in pdoc3#278

* ENH: __init__.py refactored to use f-strings

* ENH: cli.py refactored to use f-strings

* ENH: pdoc/test/__init__.py refactored to use f-strings

* ENH: pdoc/templates/html.mako refactored to use f-strings

* ENH: pdoc/html_helpers.py refactored to use f-strings

* ENH: pdoc/templates/pdf.mako refactored to use f-strings

* Update pdoc/html_helpers.py

Reworded variable name to be more meaningful
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants