Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add docformatter which formats Python docstrings to PEP 257 #267

Merged
merged 6 commits into from
Dec 15, 2023

Conversation

meliache
Copy link
Contributor

@meliache meliache commented Dec 4, 2023

Add docformatter for Python docstrings.

By default it outputs diffs but changes in-place with --in-place. On successful change it exits with an error code of 3 (found out by trial), so I had to add a formatter wrapping-script.

Initially I used --in-place with the special in-place symbol in apheleia. But now I tried an approach where I transform the diff into usable stdout using patch instead.

Related to #266 , where I had used the example of docformatter to ask how to add scripts with positive exit codes and @raxod502 showed me the phpcs solution.

@meliache meliache changed the title Add docformatter which formats Python docstrings to PEP 257 Add docformatter which formats Python docstrings to PEP 257 Dec 4, 2023
@meliache meliache force-pushed the feature/add-docformatter branch 4 times, most recently from 5ff063f to 9920042 Compare December 6, 2023 21:54
@meliache meliache force-pushed the feature/add-docformatter branch from f6d7259 to 89ea943 Compare December 6, 2023 22:16
For some reason no other built-in formatters use `file`, all use
`filepath`. So maybe this is better? Even though I don't need stdin.
And I have a loose hope this might fix the tests.
@meliache meliache marked this pull request as draft December 6, 2023 23:16
@meliache
Copy link
Contributor Author

meliache commented Dec 6, 2023

I have possible implementations of a docformatter wrapper script and both work interactively but fail the tests. I think I need some help. maybe @raxod502 has some idea?

Implementation 1: Using --in-place

Script:

#!/bin/sh
docformatter --in-place "$@"
if [ "$?" -eq 3 ]; then
	exit 0
fi

Formatter:

    (docformatter . ("apheleia-docformatter" inplace))

Error:

  error("Formatter %s modified original file in place" "docformatter")

In place is exactly what I tried to do here, supposedly apheleia has support for that, but seems it doesn't work for tests.

Implementation 1: Transformind diff to stdout

Script:

#!/bin/sh
output=$(docformatter "$@")
ret=$?
[ $ret -gt 0 ] && [ $ret -ne 3 ] && exit $ret
echo "${output}" | patch --quiet --output=-

Formatter:

    (docformatter . ("apheleia-docformatter" file))

Error:

[Errno 2] No such file or directory: '/tmp/srctestformatterssamplecodedocformatterin.py'

When testing interactively after a while I found the call to patch with standard input is very sensitive to from where it's called. It works when it's called in the same directory as the in.py but when it's in another directory it doesn't work except with a correct --strip argument. Not sure if this is what causes this weird error message.

@raxod502
Copy link
Member

It seems simpler to go with in-place in this case, and the feature is supported for a reason. Let me have a look and see if I can fix the issue with the test framework.

@raxod502
Copy link
Member

Problem 2 is solved by some bug fixes I made last week in 74d9a5d, problem 1 is solved by d74a848 just now. Sorry for how bad the test framework is.

@raxod502 raxod502 marked this pull request as ready for review December 15, 2023 02:41
Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

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

@raxod502 raxod502 merged commit 4a87523 into radian-software:main Dec 15, 2023
4 checks passed
@meliache
Copy link
Contributor Author

Thanks for fixing and merging this! I was positively surprised that this package even has a dockerized testing framework in the first place, I have contributed to enough packages with no unit tests and have no personal experience with doing them in Elisp.

@meliache meliache deleted the feature/add-docformatter branch December 15, 2023 14:19
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