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

Increase scope of docformatter and black [unitaryhack] #79

Merged
merged 21 commits into from
Jun 7, 2022

Conversation

WingCode
Copy link
Contributor

@WingCode WingCode commented Jun 4, 2022

Context for changes

  • Increase the scope of docformatter to all .py files in the repository. The behavior of docformatter has changed from checking and reporting incorrectly formatted docstring files to correct docstring in in-place.
  • The scope of black has been extended to include doc/ as well.

Example usage and tests

  • Not Applicable

Performance results justifying changes

  • Not Applicable

Workflow actions and tests

  • Not Applicable

Expected benefits and drawbacks

Expected benefits:

  • Improved documentation quality since now it will be auto-formatted to the right format.

Possible drawbacks:

  • There can arise unexpected edge cases from auto-formatting the docs in-place. Thus, PR review needs to be more careful thereby might take more time.

Related Github issues

Checklist and integration statements

  • My Python and C++ codes follow this project's coding and commenting styles as indicated by existing files. Precisely, the changes conform to given black, docformatter and pylint configurations.

  • I have performed a self-review of these changes, checked my code (including for codefactor compliance), and corrected misspellings to the best of my capacity. I have synced this branch with others as required.

  • I have added context for corresponding changes in documentation and README.md as needed.

  • I have added new workflow CI tests for corresponding changes, ensuring codecoverage is 95% or better, and these pass locally for me.

  • I have updated CHANGELOG.md following the template. I recognize that the developers may revisit CHANGELOG.md and the versioning, and create a Special Release including my changes.

@WingCode WingCode marked this pull request as ready for review June 4, 2022 14:25
@soosub
Copy link
Contributor

soosub commented Jun 5, 2022

Don't forget part 2 of the task @WingCode 👀

@codecov
Copy link

codecov bot commented Jun 5, 2022

Codecov Report

Merging #79 (923acdf) into main (d160088) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #79   +/-   ##
=======================================
  Coverage   96.28%   96.28%           
=======================================
  Files          36       36           
  Lines        2233     2233           
=======================================
  Hits         2150     2150           
  Misses         83       83           
Impacted Files Coverage Δ
flamingpy/simulations.py 96.80% <ø> (ø)
flamingpy/_version.py 100.00% <100.00%> (ø)
flamingpy/examples/surface_code.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d160088...923acdf. Read the comment docs.

@WingCode
Copy link
Contributor Author

WingCode commented Jun 5, 2022

@soosub Regarding the part 2 of the task,
I checked in my local machine whether the change from python -m docformatter --check flamingpy/*/*.py to python -m docformatter --in-place **.py was breaking any documentation. It wasn't breaking any .py files and tests were successful.

The command python -m docformatter --in-place **.py was fixing line 1 and line 2 in place. Would like me to commit the fixed setup.py after python -m docformatter --in-place **.py as part of this PR?

@soosub
Copy link
Contributor

soosub commented Jun 5, 2022

@soosub Regarding the part 2 of the task, I checked in my local machine whether the change from python -m docformatter --check flamingpy/*/*.py to python -m docformatter --in-place **.py was breaking any documentation. It wasn't breaking any .py files and tests were successful.

The command python -m docformatter --in-place **.py was fixing line 1 and line 2 in place. Would like me to commit the fixed setup.py after python -m docformatter --in-place **.py as part of this PR?

Ah, I think the second task might not have been clear @WingCode. We would also like to increase the scope of the black formatter in our repo (don't worry - it's not too much work). This is independent of the docformatter. I think it should be clear if you reread the corresponding issue #79. Hope this helps!

@soosub soosub marked this pull request as draft June 5, 2022 18:46
@soosub
Copy link
Contributor

soosub commented Jun 5, 2022

I'll convert the PR into a draft for now. Feel free to open it for review once it's ready!

@nariman87
Copy link
Contributor

nariman87 commented Jun 5, 2022

Just a general note that "**.py" is shell dependent and best would be to use more general expressions, thanks.

@WingCode
Copy link
Contributor Author

WingCode commented Jun 6, 2022

@nariman87 Good catch! How about we change to python -m docformatter -r --in-place . since it by default looks out for .py files and thereby we don't have to define the globbing pattern.

The recursive option makes sense to me, thanks.

Although, as usual, we should be careful with and test for doc files such as in ./doc/tutorials/ to not have undesired changes. The above could work but other solutions could do as well.

@WingCode
Copy link
Contributor Author

WingCode commented Jun 6, 2022

@nariman87 Thank you for the input.

If we would like to exclude some files/folders we can use --exclude with doc formatter.

We can use --check which throws up an error instead of --in-place thereby avoiding undesired changes and leaving it to the code committer to fix the issues. The reason why I changed from --check over here to --in-place was mentioned in the issue

@soosub , @nariman87 Would like me to keep it as python -m docformatter -r --check . instead and could you let me know which files/folders which we would like to exclude from docformatter check/in-place?

.github/CHANGELOG.md Outdated Show resolved Hide resolved
@soosub
Copy link
Contributor

soosub commented Jun 6, 2022

@soosub , @nariman87 Would like me to keep it as python -m docformatter -r --check . instead and could you let me know which files/folders which we would like to exclude from docformatter check/in-place?

A good rule of thumb is to always listen to @nariman87 😉 Indeed, let's go with --check. Regarding the scope, I think we can just include all .py files. Once you addressed this comment you can open the PR for review 👍🏼

@nariman87
Copy link
Contributor

Yes, definitely only do --check.

No particular file to exclude for now, we just need to review diffs carefully and if docformatter is applying an unintended change to some of the files, then exclude them.

@nariman87 nariman87 added chores Standard routines for open-source software projects workflow & tests CI tests have been modified and/or new workflows were added labels Jun 7, 2022
@WingCode WingCode marked this pull request as ready for review June 7, 2022 13:26
Copy link
Contributor

@nariman87 nariman87 left a comment

Choose a reason for hiding this comment

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

Excellent work, WingCode, one approval from me.

Let's wait for @soosub's approval and possible further comments as well.

.github/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@soosub soosub left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ilan-tz ilan-tz changed the title Increase scope docformatter to all py files [unitaryhack] Increase scope of docformatter and black Jun 7, 2022
@ilan-tz ilan-tz added the unitaryhack-accepted An unitaryHACK issue that is accepted and assigned to a developer. label Jun 7, 2022
@ilan-tz
Copy link
Collaborator

ilan-tz commented Jun 7, 2022

Congratulations on resolving our first unitaryHACK issue, @WingCode ! I will fix some small CodeFactor issues from an earlier PR and then merge.

@ilan-tz ilan-tz changed the title Increase scope of docformatter and black Increase scope of docformatter and black [unitaryhack] Jun 7, 2022
@ilan-tz ilan-tz merged commit 1363792 into XanaduAI:main Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chores Standard routines for open-source software projects unitaryhack-accepted An unitaryHACK issue that is accepted and assigned to a developer. workflow & tests CI tests have been modified and/or new workflows were added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change scope of docformatter and black 👁️
4 participants