Skip to content

eng: fix up project configuration - BNCH-112051 #221

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

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

eli-bl
Copy link

@eli-bl eli-bl commented Nov 22, 2024

These changes are only for Benchling's internal use of the package and will not be submitted to the upstream repo. They're the prerequisites for us to be able to publish versions of the prod/2.x fork.

  • Remove pdm and add a poetry-based project config. This is mostly equivalent to the existing pdm config from upstream, but with a couple of changes like publishing with a different package name. I'll probably migrate this yet again to uv, since we've been standardizing on uv elsewhere, but this was the handiest interim solution because we already were using poetry in our old fork branch.
  • Update GitHub CI to use poetry (we'll be replacing this CI later, but it's still using GitHub for now).
  • Update README and CONTRIBUTING for our own usage.

A couple of other, more obscure adjustments are noted in PR comments.

Note that there are still some references to pdm in the repo, but those are all related to the option of using pdm in the generated client package (the tool allows you to create packages that use poetry, pdm, or setup.py).

@eli-bl eli-bl marked this pull request as ready for review November 22, 2024 01:40
@eli-bl eli-bl marked this pull request as draft November 22, 2024 01:41
@eli-bl eli-bl changed the title eng: fix package ID & description - BNCH-112051 eng: fix up project configuration - BNCH-112051 Nov 22, 2024
@eli-bl eli-bl force-pushed the eli.bishop/BNCH-112051-packageid branch 2 times, most recently from c17d5b7 to f9d3461 Compare November 22, 2024 22:54
# our fork (benchling-openapi-python-client) deliberately does not match the base module name (see
# pyproject.toml)
INSTALLED_PACKAGE = "benchling-openapi-python-client"
__version__ = version(INSTALLED_PACKAGE)
Copy link
Author

Choose a reason for hiding this comment

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

As noted in a comment I added to pyproject.toml, we want our installable package to be called benchling-openapi-python-client, but we deliberately did not change the base module name to benchling_openapi_python_client, because for our own internal purposes we want to be able to treat this as a drop-in replacement for the upstream package. That means version(__package__) will not work because __package__ actually returns the base module name in this scenario, but version wants the installed package name.

repository = "https://github.com/openapi-generators/openapi-python-client"

[project.scripts]
[tool.poetry.dependencies]
Copy link
Author

Choose a reason for hiding this comment

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

The dependencies in this section are exactly equivalent to the ones that the upstream package was declaring with pdm.

ruff = ">=0.2,<0.8"
typing-extensions = ">=4.8.0,<5.0.0"

[tool.poetry.group.dev.dependencies]
Copy link
Author

@eli-bl eli-bl Nov 22, 2024

Choose a reason for hiding this comment

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

The dependencies in this section are almost the same as the ones in the upstream project, except that I added python-dotenv in order to work around an issue with the test task - see old line 125.

@@ -76,6 +91,10 @@ disallow_any_generics = true
disallow_untyped_defs = true
warn_redundant_casts = true
strict_equality = true
exclude = [
# For unknown reasons, mypy complains about this file but doesn't complain in the upstream repo
"openapi_python_client/schema/openapi_schema_pydantic/schema.py",
Copy link
Author

Choose a reason for hiding this comment

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

This one is weird - I'm using the same version of mypy and there are no code changes, no idea why it's suddenly showing an error on this file (which I think is a spurious error, the type hint looks fine to me). But that code is old and not under active development so I'm OK with adding an exclude.

[tool.pdm.scripts.test]
cmd = "pytest tests end_to_end_tests/test_end_to_end.py end_to_end_tests/functional_tests --basetemp=tests/tmp"
[tool.pdm.scripts.test.env]
"TEST_RELATIVE" = "true"
Copy link
Author

Choose a reason for hiding this comment

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

This is the reason I added python-dotenv as a dev dependency. There are some extra unit tests that only get run in CI which are enabled by setting this env var. However, we're now using taskipy instead of pdm tasks, and taskipy unfortunately has no built-in way to set env vars. So I added a .env file in the tests and added dotenv run in the test_with_coverage task below.

@eli-bl eli-bl force-pushed the eli.bishop/BNCH-112051-packageid branch 2 times, most recently from 5a8524c to f1b5332 Compare November 22, 2024 23:05
@@ -131,40 +131,40 @@ jobs:
- name: Set up Python
uses: actions/[email protected]
with:
python-version: "3.8"
python-version: "3.9"
Copy link
Author

Choose a reason for hiding this comment

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

The other CI jobs were already updated to 3.9 but I missed this one.

@eli-bl eli-bl force-pushed the eli.bishop/BNCH-112051-packageid branch from f1b5332 to 5a4c239 Compare November 22, 2024 23:10
@eli-bl eli-bl marked this pull request as ready for review November 22, 2024 23:12
@eli-bl eli-bl merged commit e16273b into prod/2.x Nov 25, 2024
18 checks passed
@eli-bl eli-bl deleted the eli.bishop/BNCH-112051-packageid branch November 25, 2024 17:31
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.

2 participants