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 'black' formatter #8

Closed
wants to merge 5 commits into from
Closed

Add 'black' formatter #8

wants to merge 5 commits into from

Conversation

eine
Copy link

@eine eine commented Jan 8, 2020

Close #7

This PR is a proposal to use psf/black as the formatter for this project. It includes a tox configuration file and a GitHub Actions workflow to check the format after each push/PR.

Note that I did not format the code. This can be done with tox -e py37-fmt.

Currently, there is an error when installing this package. See https://github.com/eine/pyVHDLParser/runs/380246563

FTR, see related discussion in VUnit: VUnit/vunit#554

@Paebbels
Copy link
Owner

Paebbels commented Jan 8, 2020

Currently, I do not understand why we need another configuration and packaging format called tox, that duplicates setup.py and requirements.txt.

@eine do you have an introduction to tox and why tox does not replace the other two formats. Moreover tox is not so widely supported like requirements.txt

@eine
Copy link
Author

eine commented Jan 8, 2020

I'd say that tox is a virtual environment manager and a test runner: https://tox.readthedocs.io/en/latest/#what-is-tox. We might consider it to be 'configuration', but it is by no means a duplicate of setup.py. Precisely, tox installs the package using regular Python mechanism, so setup.py is used. This is to say that the error is likely due to a missing package_data field in the setup. In this case, tox is just triggering an error that other users trying to get the package from PYPI might find.

Precisely, the proposed tox file has a two purposes:

  • Select which version of Python to use, should you have multiple versions installed on your host. Currently, it is irrelevant for the GitHub Actions workflow, because a single version is tested (3.7). Hence, if you want, I can just remove tox.ini and use python -m black ./ --check instead.
  • Install black as a "development dependency", not required for the package but only for testing. I don't know whether it is possible to define this kind of dependency in setup.py. Alternatively, a requirements.dev file can be added, which developers should install.
    • Note that this "development dependecy" concept is applied to other "tests" such as running sphinx. Using requirements files requires to have a separate file for each set of dependencies. Tox allows to define all of them in the same place.
      • A downside of this is that tox will create a clean environment and install the dependencies for each run. I don't know if it is possible to tell tox not to use a virtualenv, but to run with the packages installed on the host.

@Paebbels
Copy link
Owner

Paebbels commented Jan 8, 2020

@eine I saw with Python-based venv CI platforms that they have a use system packages checkbox or option. This means use pre-installed global packages if not defined with other versions or additional packages in a venv description.

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@Paebbels
Copy link
Owner

Paebbels commented Jan 9, 2020

To what branch will black push the changes?

@eine
Copy link
Author

eine commented Jan 9, 2020

To what branch will black push the changes?

None at all. In the CI job, black is executed with option --check. It will report which files it would modify, but it will not apply changes. Developers are expected to execute tox -e py38-fmt locally, to have sources formatted.

If you want, I can run it locally once and push all the format modifications in a single commit. Or, you can do it yourself: pip install black && black . will produce the same result.


with open("README.md", "r") as file:
long_description = file.read()

projectName = "pyVHDLParser"

rfile = Path("requirements.txt")
Copy link
Owner

Choose a reason for hiding this comment

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

Using Pathlib is even much simpler :).

-from pathlib import Path, PurePath
+from pathlib import Path

projectName = "pyVHDLParser"

rfile = Path("requirements.txt")
-root = PurePath(__file__)
-if not Path.is_file(rfile):
-  rfile = Path(root.parent, projectName + ".egg-info", "requires.txt")
-  if not Path.is_file(rfile):
+root = Path(__file__)
+if not rfile.is_file():
+  rfile = root.parent / (projectName + ".egg-info") / "requires.txt"
+  if not rfile.is_file():
		exit(1)
requirements = []
-with open(rfile) as file:
+with rfile.open() as file:

Pathlib is fully OO.

  • They overloaded the / operator for path concatenation :).
  • is_file is a method, not a classmethod -> data knows best how to test itself.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot! I updated the PR accordingly...

@Paebbels Paebbels force-pushed the master branch 4 times, most recently from 3c716c8 to fb38242 Compare September 6, 2020 22:42
@Paebbels
Copy link
Owner

@eine after seeing how dumb I need to reject this PR.

Maybe with another formatter (supporting rules and don't touch regions), but NOT black.

@Paebbels Paebbels closed this Jun 30, 2021
@eine eine deleted the add-black branch June 30, 2021 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code formatting style.
2 participants