Refactor of the formatter to scale better later on #9
Conversation
Pierre-Sassoulas
force-pushed
the
capitalize-first-letter
branch
2 times, most recently
from
3320b7b to
28db19b
Compare
|
I like this approach! |
Pierre-Sassoulas
force-pushed
the
capitalize-first-letter
branch
from
28db19b to
a3ac539
Compare
Pierre-Sassoulas
force-pushed
the
formatter-refactor
branch
from
259278b to
69ec5c9
Compare
Pierre-Sassoulas
changed the title
pydocstringformatter/formatting.py
Outdated
| return new_string | ||
|
|
||
|
|
||
| class ClosingQuotesFormatter(Formatter): |
There was a problem hiding this comment.
Shouldn't this be a StringFormatter as well?
There was a problem hiding this comment.
return self._format_multiline_ending_quotes(tokeninfo) returns a token
There was a problem hiding this comment.
It does, but only ever changes the string. It's a bad design that gets shown by this wonderful refactor! 😄
There was a problem hiding this comment.
Hmm but it needs the token and not the string to work. Maybe treat_string needs to take a token as input ?
There was a problem hiding this comment.
Or we could initialize the formatters for each loop and the token become a class attribute.
There was a problem hiding this comment.
Or we could initialize the formatters for each loop and the token become a class attribute.
That sounds like a large impact on performance. Would probably still be manageable, but I like how quick the tool runs at the moment.
There was a problem hiding this comment.
I've refactored treat_string, the result does look cleaner.
|
@Pierre-Sassoulas Just something I found while reading the PEPs (and to be added at a later point) one-liner's and summary lines should be sentences ending with a point. I have not done that myself for some of the docstrings I already wrote, but we might as well start doing this ourselves now. |
|
I can remove the |
Would be good I think! I hate having to scroll down two pages on a Github repository because of all the config files, that's why I'd like to enforce not having a I should add a |
Pierre-Sassoulas
force-pushed
the
formatter-refactor
branch
from
11f0c21 to
5307469
Compare
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
for more information, see https://pre-commit.ci
It's actually nice to be on a project with a bug to feature ratio of 0%, how do you do that ? 😄 |
Rigorous reviews of all PRs 😄 |
2 participants
This is a proposition so it's easier to add Formatter later on and remove some duplicated calls. (Based on #8)