s2p
s2p copied to clipboard
Support black format for all Python files
Description
Dear the team of s2p: First, this project is awesome. It's the photogrammetry project that is thorough and comprehensive to follow for everyone who want to enter this field. I propose that this project could follow the PEP 8 – Style Guide for Python Code to make it more friendly and easier to read. Therefore, I would like to introduce the open source python package black into this project. black is the uncompromising Python code formatter, which can help us to save time and mental energy for more important matters and be more efficient in writing code. Developers can contribute to this project in more uniform coding style.
Type of change
The changes of this PR are:
- [x] Add black as an extra requirement in
setup.py
. Developers can install thisblack
in development bypip install -e .[dev]
.
extras_require = {"test": ["pytest", "pytest-cov", "psutil"], "dev": ["black"]
- [x] Format every Python files in
s2p
,tests
, andutils
withblack
automatically without any manual operation. Thus, the logic of the program does not change at all. There are 41 Python files ins2p
,tests
,utils
andsetup.py
. 40 Python files have been formatted because/utils/__init__.py
is empty.
How Has This Been Tested?
This project has been tested with the command, pytest tests/
, and all tests pass.
Checklist:
- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my own code
- [x] My changes generate no new warnings
- [x] Existing unit tests pass locally with my changes
@gfacciol Could you please review this PR? Thank you.
Hi @lionlai1989, thanks for contributing!
Is there a way to configure black so that the code style stays closer to that of the original codebase? I think that in its current state this pull request is introducing too many style changes, which may obfuscate code history.
Hey @carlodef Thanks for replying. Here are some points I can think of for this PR.
- In general, yes. Black supports configuration to tailor to each individual project. But also it's recommended not to change the configuration of Black. Because the whole purpose of using black is that it resolves all coding-style issues in an uncompromising way while following the PEP-8 guide.
- However, I agree with you that this PR introduces too many style changes and could obfuscate commit history. So, maybe you can point out which style we should keep to make this PR not so confusing and which style is OK to be changed. From my perspective, it looks like converting single quote
'
to double quote"
really affects many lines of code. So, maybe keeping single quote is preferable? - Yeah, incorporating black into an existing project sometimes brings inconvenience to the existing codebase. But I sincerely believe that the pain is temporal and will fade out, and this PR will be rewarding in long term.
- Lastly, do you think it will help if I add style-guide section in README.txt?
Thank you.