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

Write access #215

Open
MattiasMTS opened this issue Sep 3, 2022 · 5 comments
Open

Write access #215

MattiasMTS opened this issue Sep 3, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@MattiasMTS
Copy link

Hello Impedance World

I was about to make a slight contribution to the source code by adding the following:

  • Poetry (proper python package manager, no more requirements.txt files!) and at the same time update the github workflows for easier CI/CD stuff using poetry.
  • pre-commit config, add some neat configuration that checks your code on each commit before pushing. E.g. black formatting, flake8, trimming white spaces, etc.
  • Some instructions for Dev Setup using poetry.

Let me know if you wish to have this added! I will then make a PR for it, just need the access 😋
poetry: https://python-poetry.org/docs/libraries/

@MattiasMTS MattiasMTS added the enhancement New feature or request label Sep 3, 2022
@mdmurbach
Copy link
Member

Hi Mattias, thanks for reaching out! I think (?) you are able to make a pull request without direct write access by forking into your own space and then requesting to merge your changes back in: https://github.com/ECSHackWeek/impedance.py/blob/main/CONTRIBUTING.md#repository-setup

I'd be curious to understand what advantages you see from using poetry vs. the current setup.py/requirements.txt/etc.? Similarly, we've been using flake8 to enforce PEP8 (via GitHub Actions) for the code (in my experience black often introduces a ton of purely whitespace changes that can make PRs more challenging to review although if everyone were to use it and we made a huge one-time shift to black output I suppose it could work). In general though, I usually would prefer to require the fewest dependencies as possible such that the bar to contributing is as easy as possible.

@BGerwe or others: you use poetry at all? Any thoughts?

@MattiasMTS
Copy link
Author

Thanks for the feedback, Matt!

poetry

You can still use requirements.txt with poetry by simply using the poetry export command. However, why poetry is starting to become a standard in the modern stacks are mainly because of:

  1. Allow separation between dev-dependencies and prod-dependencies. Similarly using requirements-dev.txt and requirements.txt.
  2. Easier virtual environment management. Instead of using venv, you can now simply use poetry shell and then poetry install to install all the dependencies (dev or prod) defined in the pyproject.toml file. If you don't have an automatic environment switcher, this is now easier to activate your virtual environment by just typing poetry shell again rather than the source .. path stuff.
  3. Easier dependencies update. Using e.g. poetry update checks if the project's dependencies and their sub-dependencies automatically align. If it doesn't, it will either roll back or alarm you with future-warning. You can combine this with poetry show [parameter] to really see how your package's dependencies.
  4. Easy to build and deploy your project by using poetry build. This combined with some github actions artefacts allows you to automatically dump your .whl instalment package in the release notes.
  5. It now supports automatic publish to PyPi (also private repos) by using the poetry publish command. For release, you could easily write a github actions that is triggered by merging to main (if that is the workflow).
  6. Easily structure and configure your project's linting rules in the pyproject.toml file. Here, you can add a section called flake8 and list any CLI kwargs that it should understand. Ergo, all your project's structures would be defined in this pyproject.toml file.

pre-commit

Super nice that you are using a linting in the github actions. You should have this as well, but my experience with this is that it can lead to unnecessary commits. I.e. if the github actions fail, then you, the user, would have to run the linting rules you have defined in the github actions CI/CD locally, then push those changes. With pre-commit it does exactly that for you, before committing. For example, let's say you have a pre-commit-config.yaml, where you only have flake8 rules. During commit, the pre-commit will run this behind the scenes and check it for you. If the pre-commit fails on flake8, it will then say "fail" and not commit those changes. Subsequently, it will tell you where it failed and allows you to fix it before pushing it. Together with black, you can automatically set up pre-commit w.r.t. flake8 (PEP-8 conv.) so it formats your code on commit and then updates it for you. I'm attaching a picture so you can see what this looks like.
image

Let me know if you wanna have a quick meeting or so, where I can show a demo of how it would look in action! 😋

@mdmurbach
Copy link
Member

Better dependency management definitely makes sense to me. My only concern with the pre-commit hooks is that it adds an extra step to getting new contributors setup (at least that was my experience with a few work projects), but as long as we can keep the CONTRIBUTING.MD super approachable I'd be open to the improvements in maintainability :)

@MattiasMTS
Copy link
Author

Absolutely.
I will separate the features (poetry and pre-commit) into separate PRs. Starting with Poetry. Do you want me to cover #217 for the poetry PR as well?

@mdmurbach
Copy link
Member

Sure, that'd be great! I've already added a PYPI_API_TOKEN secret to GitHub

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: To prioritize
Development

No branches or pull requests

2 participants