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

Support PEP 621 #266

Merged
merged 8 commits into from
Sep 3, 2024
Merged

Support PEP 621 #266

merged 8 commits into from
Sep 3, 2024

Conversation

albireox
Copy link
Member

@albireox albireox commented Sep 3, 2024

This PR adds support for PEP 621 (a.k.a pyproject.toml) and tries to clean up the development environment a bit.

What it does:

  • Adds a valid pyproject.toml file with setuptools as build system and removes setup.cfg and setup.py.
  • Cleans and updates the dependencies a bit, by moving some things that are not normally needed to optional dependency groups.
  • Adds configuration in pyproject.toml for ruff, flake8, and isort. The ruff and flake8 configurations are set up to match the same .flake8 file we had. I did some very minor changes to a few files to make both ruff check and flake8 pass, but most problematic files are still being ignored.
  • Fixes the tests to pass.
  • Adds linting and test workflows to GitHub.
  • Deletes more lines than it adds.

What it does NOT do:

  • Does not settle on an opinionated project management library like poetry or uv.
  • Does not go over the dependencies and updates or removes them where not needed.
  • Does not expand or really check that the tests make sense (but it's nice to have them back).
  • Does not format the files to follow a formatting guide (e.g., black or ruff).

Some of those are discussed in #267.

Closes #251
Closes #67

@albireox albireox mentioned this pull request Sep 3, 2024
12 tasks
@albireox albireox mentioned this pull request Sep 3, 2024
Comment on lines +1 to +4
name: Test

on:
push:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to split out build and test into two separate workflows? Or if not, split build and test into two separate jobs in this workflow?

Do we want to add any tag and release workflows?

Copy link
Member Author

Choose a reason for hiding this comment

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

With this new version there are 3 workflows: test, lint, and release. release (which builds the source and wheel packages and uploads them to PyPI) has been there for some time and I have not changed it here. The lint workflow is what it used to be called pythonapp. The test workflow is new and replaces the Travis setup.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, that makes sense.

Comment on lines +34 to +48
- name: Install Postgresql
uses: ikalnytskyi/action-setup-postgres@v6
with:
username: postgres
id: postgres

- name: Install dependencies
run: |
uv pip install --system -e .[dev]

- name: Test with pytest
run: |
pytest -s
env:
PGSERVICE: ${{ steps.postgres.outputs.service-name }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised the tests just worked with these additions. I can finally delete the outdated ghtests branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was also surprised that this was fairly simple. I also had to change the DatabaseJanitor a bit since it has changed in newer versions of pytest_postgresql but nothing too dramatic.

]

[tool.ruff]
line-length = 99
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can only dream of line-length = 120

Copy link
Member Author

Choose a reason for hiding this comment

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

We can have that discussion for sdssdb 1.0. It should come with a committed VSCode config that sets the editor font size to 8pt.

Comment on lines 21 to 28
# only run peewee tests
parser.addoption('--peewee', action='store_true', default=False,
help='Only run tests for peewee dbs')
parser.addoption(
"--peewee",
action="store_true",
default=False,
help="Only run tests for peewee dbs",
)

Copy link
Collaborator

@havok2063 havok2063 Sep 3, 2024

Choose a reason for hiding this comment

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

I'll just comment here but no need to change it. This is my other grievance with formatters. Not a fan at all when they split inputs onto new lines

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's a problem right now. We have a linter but not a formatter. My default VSCode config will apply ruff format on save, which maybe shouldn't for this repo, or should do it only on files that one wants to keep formatted. Something to discuss for 1.x.

Copy link
Collaborator

@havok2063 havok2063 left a comment

Choose a reason for hiding this comment

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

This PR looks good to me. It's a long needed improvement.

@albireox albireox merged commit b0fd161 into main Sep 3, 2024
2 checks passed
@albireox albireox deleted the albireox/pep-621 branch September 3, 2024 17:02
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.

Migrate to pyproject.toml Move tests to GitHub Actions
2 participants