Feature: two-pass formatting of the tokens #173

jspaezp · 2022-09-16T10:00:14Z

Discussed here:
#169

Things to iron out:

should arguments be added to enable it? (--num-passes ... --no-check ????) .... I vote no arguments ...
should the error be more user-friendly?
Regarding the failing tests ... option A. Fix the formatters so they become compatible, option B. Fix the tests so the incompatible formatters do not get called together (also change the defauts of just adding the --style"numpydoc" to prevent automatic collisions ...). LMK what you think.
testing ... Right not by "playing with it" it works but a better unit testing is required.

I haven't looked at your branch as I am on mobile, but I'm highly in favour of this change. I'll gladly help review a PR but sadly won't have time to work on this myself this week.

Some pointers to consider:
* I think the maximum run count should be 2.

* After the second run, a third run should occur. If we still create changes (note that these should ignore the --write flag but should only check if there are any potential change) we need to emit an error. `black` has a very elegant system where after there is an inconsistency aftwr 2/3 runs a pre-regenerated bug report is send to stdout. This can then be copied to an issue in GitHub which would allow us to resolve the issue.

* We should test the issue template. So we need to write two incompatible test formatters and write a test where they fail to find a solution.
That's all I can think of for now. Let me know if you have any questions! 😊

Issue template to report the problems

Current error:

>           raise RuntimeError(msg)
E           RuntimeError: Conflicting formatters: numpydoc-section-spacing, closing-quotes
E           --- numpydoc-section-spacing vs closing-quotes /private/var/folders/5j/x2qvf77j1m7736lccs2xt6c40000gn/T/pytest-of-sebastianpaez/pytest-187/test_formatting_numpydoc_numpy0/test_file.py
E           +++ numpydoc-section-spacing vs closing-quotes /private/var/folders/5j/x2qvf77j1m7736lccs2xt6c40000gn/T/pytest-of-sebastianpaez/pytest-187/test_formatting_numpydoc_numpy0/test_file.py
E           @@ -1,2 +1 @@
E           -"""A docstring.
E           -    """
E           +"""A docstring."""

.gitignore

pydocstringformatter/run.py

jspaezp · 2022-09-16T23:16:36Z

Looking at the conflicting formatters ....

NumpydocSectionSpacingFormatter collides with ClosingQuotesFormatter

NumpydocSectionSpacingFormatter
https://github.com/jspaezp/pydocstringformatter/blob/858c1dabe6880e7367f692674bdc731eae398fe1/pydocstringformatter/_formatting/formatters_numpydoc.py#L104
generates:

'"""\n    A docstring\n    """'

ClosingQuotesFormatter
https://github.com/jspaezp/pydocstringformatter/blob/858c1dabe6880e7367f692674bdc731eae398fe1/pydocstringformatter/_formatting/formatters_default.py#L119
generates:

'"""A docstring"""'

Looking at references

Numpydoc differentiates between short summary and extended summary.
https://numpydoc.readthedocs.io/en/latest/format.html#short-summary

where I think the correct out would be ...

# CURRENT
'"""\n    A docstring\n    """'

# Expected
'"""A docstring\n\n    """'

Which for sure would be incompatible....

Solution 1: Make NumpydocSectionSpacingFormatter not modify anything if there is only one section.
Solution 2: Make NumpydocSectionSpacingFormatter by making it call the ClosingQuotesFormatter on its current output.
Solution 2: Add a Formatter.conflcting atttribute (also entails fixing the tests so that one of the args is disabled)
- Op1: and check for conflicting formatters when calling _Run, warn about the behavior and disable all conflciting formatters.
- Op2: same but error out instead of warning (also entails changing the defaults)

Extras:

Improve documentation for closing-quotes to make the behavior more explicit.

  --beginning-quotes, --no-beginning-quotes
                        Activate or deactivate beginning-quotes: Fix the position of the opening quotes. Styles: default. (default: True)
  --closing-quotes, --no-closing-quotes
                        Activate or deactivate closing-quotes: Fix the position of the closing quotes. Styles: default. (default: True)

jspaezp · 2022-09-17T01:02:38Z

tests/data/format/linewrap_summary/function_docstrings.args
breaks as well ...

(expectation left, actual right)

and adds two newlines...

because:

INFO root:run.py:225 linewrap-full-docstring modified
INFO root:run.py:225 split-summary-body modified <- here the additional newline is added
INFO root:run.py:223 linewrap-full-docstring run <---- here summary splits incorrectly

SplitSummaryAndDocstringFormatter adds the newline here:
https://github.com/jspaezp/pydocstringformatter/blob/af502f1ba70611630235aa1fe0854c883ecd84b2/pydocstringformatter/_formatting/formatters_pep257.py#L10

https://github.com/jspaezp/pydocstringformatter/blob/c0ea048e7e2cbd883bd32015bad317f7bf39258b/pydocstringformatter/_formatting/base.py#L136

splits the summary incorrectly

Edit

I think this is a feature and not a bug ...

why: because it reconciles "max_summary_lines" and "max_line_length" ... since the summary first gets broken due to their max-length and then the space gets added because the max_summary_lines is 1 !!!

pydocstringformatter/run.py

codecov · 2022-09-17T04:15:21Z

Codecov Report

Merging #173 (5799b7f) into main (93b15ca) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #173   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        23    +1     
  Lines          640       684   +44     
=========================================
+ Hits           640       684   +44

Impacted Files	Coverage Δ
pydocstringformatter/_testutils/__init__.py	`100.00% <100.00%> (ø)`
...ocstringformatter/_testutils/example_formatters.py	`100.00% <100.00%> (ø)`
pydocstringformatter/_utils/__init__.py	`100.00% <100.00%> (ø)`
pydocstringformatter/_utils/exceptions.py	`100.00% <100.00%> (ø)`
pydocstringformatter/_utils/issue_template.py	`100.00% <100.00%> (ø)`
pydocstringformatter/run.py	`100.00% <100.00%> (ø)`

pydocstringformatter/run.py

jspaezp · 2022-09-17T07:16:17Z

@DanielNoord I think I am at the point where some design decisions have to be made.
LMK what you think (no rush)

DanielNoord

Thanks a lot @jspaezp there are many good changes in here.

I did a first review. Let me know what you think!

pydocstringformatter/_formatting/_utils.py

pydocstringformatter/run.py

tests/data/format/linewrap_summary/function_docstrings.args

tests/test_conflicting_formatters.py

tests/test_formatting_utils.py

DanielNoord · 2022-09-18T10:24:33Z

Looking at the conflicting formatters ....

NumpydocSectionSpacingFormatter collides with ClosingQuotesFormatter

# CURRENT
'"""\n    A docstring\n    """'

# Expected
'"""A docstring"""'

I think the Expected should be as above. This is what PEP256 mandates for single line docstrings. Numpy refers to the PEP in its introduction so I think they will accept that this is indeed the correct "output" for these examples.

Would you wan to create an issue/PR to fix this?

I think this is a feature and not a bug ...

why: because it reconciles "max_summary_lines" and "max_line_length" ... since the summary first gets broken due to their max-length and then the space gets added because the max_summary_lines is 1 !!!

Agreed. Perhaps we should switch the order around? So the wrapping is done before the splitting?

github-actions · 2022-09-22T08:34:20Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

github-actions · 2022-09-22T13:12:22Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

github-actions · 2022-09-22T13:13:23Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

github-actions · 2022-09-24T20:16:27Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

github-actions · 2022-09-24T20:19:37Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

pydocstringformatter/_utils/issue_template.py

pydocstringformatter/run.py

tests/test_conflicting_formatters.py

github-actions · 2022-09-24T20:37:40Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

tests/test_conflicting_formatters.py

github-actions · 2022-09-24T20:59:14Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

DanielNoord

Looks much better as a context manager!

pydocstringformatter/run.py

tests/test_conflicting_formatters.py

github-actions · 2022-09-24T21:04:06Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

github-actions · 2022-09-24T21:05:20Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

github-actions · 2022-09-24T21:05:24Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

github-actions · 2022-09-24T21:05:29Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

DanielNoord · 2022-09-24T21:07:05Z

tests/test_conflicting_formatters.py

+def patched_run(formatters: list[Formatter]) -> Generator[type[_Run], None, None]:
+    """Patches the _Run class to use only the provided formatters."""
+    old_formatters = _formatting.FORMATTERS
+    try:


Why are we in a try here?

To be honest, its to be safe. it makes sure the cleanup happens if there are un-handled errors while yielding the patched run (which I'm not sure it it would affect all other tests).

https://docs.python.org/3/library/contextlib.html

Hm, for now I'd say let's remove this. If any error occurred while patching formatters (so while we run this test) the test (and test suite) will fail anyway. It's not necessary to guarantee a successful test suite run as if we raise we will fail anyway.

I did learn something new today though, that's always nice 😄

Right on!
Alright, it works after removing it in 862356d

github-actions · 2022-09-24T21:25:09Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

DanielNoord

I'm going to give this one final review tomorrow when I'm "fresh", but I think we ironed out all the kinks 😄 🎉

Co-authored-by: J. Sebastian Paez <[email protected]>

github-actions · 2022-09-25T08:33:32Z

According to the primer, this change has no effect on the checked open source code. 🤖🎉

DanielNoord

@jspaezp I rebased and reordered the commit history to make it a bit cleaner. I also did some changes to capitalisation and changed changers to be a set[str] instead of list[str]. If you agree with the current state of the PR I think we can merge 😄

jspaezp · 2022-09-25T14:33:46Z

those seem like good changes!

This comment has been minimized.

Sign in to view

jspaezp commented Sep 16, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jspaezp commented Sep 16, 2022

View reviewed changes

pydocstringformatter/run.py Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jspaezp marked this pull request as draft September 17, 2022 01:12

This comment has been minimized.

Sign in to view

jspaezp commented Sep 17, 2022

View reviewed changes

pydocstringformatter/run.py Outdated Show resolved Hide resolved

jspaezp commented Sep 17, 2022

View reviewed changes

pydocstringformatter/run.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jspaezp marked this pull request as ready for review September 17, 2022 05:15

This comment has been minimized.

Sign in to view

Pierre-Sassoulas added the enhancement New feature or request label Sep 17, 2022

DanielNoord requested changes Sep 17, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

jspaezp requested a review from DanielNoord September 24, 2022 20:21

DanielNoord requested changes Sep 24, 2022

View reviewed changes

DanielNoord reviewed Sep 24, 2022

View reviewed changes

tests/test_conflicting_formatters.py Outdated Show resolved Hide resolved

DanielNoord requested changes Sep 24, 2022

View reviewed changes

pydocstringformatter/run.py Outdated Show resolved Hide resolved

tests/test_conflicting_formatters.py Outdated Show resolved Hide resolved

tests/test_conflicting_formatters.py Outdated Show resolved Hide resolved

DanielNoord reviewed Sep 24, 2022

View reviewed changes

DanielNoord linked an issue Sep 24, 2022 that may be closed by this pull request

Solve/diagnose conflicting formatters #169

Closed

DanielNoord reviewed Sep 24, 2022

View reviewed changes

DanielNoord self-requested a review September 24, 2022 21:31

DanielNoord and others added 4 commits September 25, 2022 10:19

Create the AddBFormatter example formatter

98cdd94

Co-authored-by: J. Sebastian Paez <[email protected]>

Create UnstableResultError

282cf7f

Co-authored-by: J. Sebastian Paez <[email protected]>

Add create_gh_issue_template utility

ad9f251

Co-authored-by: J. Sebastian Paez <[email protected]>

Do a double (or triple) pass on each file

5799b7f

Co-authored-by: J. Sebastian Paez <[email protected]>

DanielNoord force-pushed the feature/twopass branch from 862356d to 5799b7f Compare September 25, 2022 08:32

DanielNoord approved these changes Sep 25, 2022

View reviewed changes

DanielNoord merged commit 8546b86 into DanielNoord:main Sep 25, 2022

jspaezp deleted the feature/twopass branch September 25, 2022 16:45

Feature: two-pass formatting of the tokens #173

Feature: two-pass formatting of the tokens #173

Conversation

jspaezp commented Sep 16, 2022 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jspaezp commented Sep 16, 2022 • edited Loading

jspaezp commented Sep 17, 2022 • edited Loading

Edit

I think this is a feature and not a bug ...

This comment has been minimized.

codecov bot commented Sep 17, 2022 • edited Loading

Codecov Report

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jspaezp commented Sep 17, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

DanielNoord commented Sep 18, 2022

I think this is a feature and not a bug ...

This comment has been minimized.

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

github-actions bot commented Sep 24, 2022

github-actions bot commented Sep 24, 2022

github-actions bot commented Sep 24, 2022

github-actions bot commented Sep 24, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2022

github-actions bot commented Sep 24, 2022

github-actions bot commented Sep 24, 2022

github-actions bot commented Sep 24, 2022

DanielNoord Sep 24, 2022

Choose a reason for hiding this comment

jspaezp Sep 24, 2022

Choose a reason for hiding this comment

DanielNoord Sep 24, 2022

Choose a reason for hiding this comment

jspaezp Sep 24, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

jspaezp commented Sep 25, 2022

jspaezp commented Sep 16, 2022 •

edited

Loading

jspaezp commented Sep 16, 2022 •

edited

Loading

jspaezp commented Sep 17, 2022 •

edited

Loading

codecov bot commented Sep 17, 2022 •

edited

Loading