s2p icon indicating copy to clipboard operation
s2p copied to clipboard

Support black format for all Python files

Open lionlai1989 opened this issue 2 years ago • 3 comments

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 this black in development by pip install -e .[dev].
extras_require = {"test": ["pytest", "pytest-cov", "psutil"], "dev": ["black"]
  • [x] Format every Python files in s2p, tests, and utils with black automatically without any manual operation. Thus, the logic of the program does not change at all. There are 41 Python files in s2p, tests, utils and setup.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

lionlai1989 avatar Jul 29 '22 12:07 lionlai1989

@gfacciol Could you please review this PR? Thank you.

lionlai1989 avatar Aug 02 '22 03:08 lionlai1989

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.

carlodef avatar Aug 02 '22 08:08 carlodef

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.

lionlai1989 avatar Aug 02 '22 10:08 lionlai1989