DT-7143 use uv and ruff #26
Conversation
njmei
force-pushed
the
DT-7143-use-uv-and-ruff
branch
from
f201426 to
9fe5e77
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
- Coverage 87.98% 87.97% -0.01%
==========================================
Files 37 37
Lines 3347 3361 +14
Branches 485 489 +4
==========================================
+ Hits 2945 2957 +12
- Misses 304 307 +3
+ Partials 98 97 -1
🚀 New features to boost your workflow:
|
|
|
||
| install-force: clean-install-stamp install ## Force install package dependencies | ||
| install-dev: .uv ## Installs development (dev/lint) related dependencies |
There was a problem hiding this comment.
I understand you are trying to make these targets more granular, but for the sake of consistency with all our other packages, does it not make sense to have install make target install all of the dev dependencies? It is already installing dev, by default. especially since this makefile is used only by internal devs.
There was a problem hiding this comment.
if you really want to have a target for only installing the source, could we make an explicit target that calls uv sync --frozen --no-default-groups (since make install is already installing half of these dev deps already).
|
|
||
| tox: $(INSTALL_STAMP) ## Run Test in tox environment | ||
| $(VENV_BIN)/tox | ||
| pytest: install ## Run test (pytest) |
There was a problem hiding this comment.
based on the way targets are defined above, I dont think pytest or any test dependencies would be installed properly based on this target dependency.
There was a problem hiding this comment.
I stand corrected. the dev dependency group is always synced by default. This can be updated by updating the default-groups configuration field but the default for default-groups is dev https://docs.astral.sh/uv/concepts/projects/dependencies/#dependency-groups
| [project.optional-dependencies] | ||
| # For dev dependencies: https://peps.python.org/pep-0735/ | ||
| # See also: https://docs.astral.sh/uv/concepts/projects/dependencies/#dependency-groups | ||
| [dependency-groups] |
There was a problem hiding this comment.
maybe it makes sense to specify default-groups to include lint in addition to dev?
default-groups=['dev', 'lint']
| logger.error(error_msg) | ||
| raise AWSError(error_msg) | ||
| try: | ||
| region = AWSRegion(region) | ||
| assert region is not None |
There was a problem hiding this comment.
why would this ever not be none?
| @@ -144,8 +144,9 @@ def fix_collisions( | |||
| other (ConditionExpressionComponents): the other Expression Condition to modify | |||
|
|
|||
| Returns: | |||
| ConditionExpressionComponents: The other, as a modified non-overlapping Expression Condition | |||
| """ | |||
| ConditionExpressionComponents: The other, as a modified | |||
There was a problem hiding this comment.
did ruff split this docstring in the following way? I thought that the noqa: E501 didnt ignored the line length.
| if regions: | ||
| normalized_regions = regions |
There was a problem hiding this comment.
could this not be done as a list comprehension? normalized_regions = regions or [None]
| normalized_regions: list[str] | list[None] = [None] | ||
| if regions: | ||
| normalized_regions = regions | ||
|
|
| @@ -345,7 +347,8 @@ def __post_init__(self): | |||
| imageIds=[dict(imageDigest=self.image_digest)], | |||
| ) | |||
|
|
|||
| if len(response["images"]) == 0 or "imageManifest" not in response["images"][0]: | |||
| no_images = len(response["images"]) == 0 | |||
There was a problem hiding this comment.
did ruff complain about the previous implementation?
| if tags is None: | ||
| tags = [] |
There was a problem hiding this comment.
did ruff complain about the previous implementation?
| images = sorted(repo.get_images("TAGGED"), key=lambda _: _.image_pushed_at) | ||
|
|
||
| def get_image_push_time(image: ECRImage) -> datetime: | ||
| if image.image_pushed_at is None: |
There was a problem hiding this comment.
will this be none ever?
This PR addresses DT-7143: https://alleninstitute.atlassian.net/browse/DT-7143
It replaces
black, andisortdependencies withruffand also starts usinguvfor our dependency/env managerI would recommend browsing by commit
Notes:
[project.optional-dependencies](e.g.extras) for dev dependencies and INSTEAD use[dependency-groups]string_processingpreview style astral-sh/ruff#6936