Ruff format and lint #52
Conversation
| @@ -1,50 +1,57 @@ | |||
| [tool.poetry] | |||
| [project] | |||
There was a problem hiding this comment.
switched this over to using a more generic format, which will make the transition to using uv, if such a transition occurs, smoother.
| asteval = "^0" | ||
| click = "^8" | ||
| curies = "*" | ||
| duckdb = { version = "^0", optional = true } |
There was a problem hiding this comment.
moved to optional-dependencies section
| "tox>=4.24.2", | ||
| ] | ||
|
|
||
| [tool.poetry.group.dev.dependencies] |
There was a problem hiding this comment.
this is also in the optional-dependencies section now
ialarmedalien
requested review from
cmungall,
pkalita-lbl and
sierra-moxon
and removed request for
cmungall and
sierra-moxon
| types: | ||
| - opened | ||
| - reopened | ||
| - synchronize | ||
| - ready_for_review |
There was a problem hiding this comment.
run linting and test on all PRs in a non-draft state, regardless of what the base branch is
There was a problem hiding this comment.
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Files not reviewed (1)
- tox.ini: Language not supported
Comments suppressed due to low confidence (1)
tests/test_transformer/test_object_transformer.py:364
- [nitpick] Consider renaming the parameter 'ex' to 'explicit' for clarity and consistency with its usage in the test signature.
def mk(mv: bool, ex: bool = False) -> SchemaDefinition:
There was a problem hiding this comment.
@ialarmedalien - Please see comments reflecting my poetry 1->2 migration experiences/failures 😉 .
There was a problem hiding this comment.
Inject poetry dynamic versioning into pipx-installed poetry:
- name: Install Poetry
run: |
pipx install poetry
pipx inject poetry poetry-dynamic-versioning| run: tox -e lint | ||
|
|
||
| test: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Below, also inject poetry dynamic versioning into pipx-installed poetry:
- name: Install Poetry
run: |
pipx install poetry
pipx inject poetry poetry-dynamic-versioningThere was a problem hiding this comment.
Versioning isn't really needed to run the tests, is it?
There was a problem hiding this comment.
From the run logs a version 0.0.0 is installed. While it does not break tests, you will also not notice if dynamic versioning breaks or is configured wrong, if version 0.0.0 is "good enough".
There was a problem hiding this comment.
IMO it'd be better to have an explicit test that ensures that versioning is working as expected, which will error out if the versioning plugin hasn't done its job. It can be very simple -- e.g. grepping the version info from one of the places where the plugin automatically updates it, and ensuring that it isn't 0.0.0. I don't want to have to check run logs unless I'm looking for reasons why tests failed.
There was a problem hiding this comment.
I don't think it's worth adding such a workflow or test because it's something that can easily be checked the next time that a release is made, and having a workflow that tests the functionality as part of the linkml-map test suite is essentially testing other peoples' code.
ialarmedalien
force-pushed
the
ruff_format_and_lint
branch
2 times, most recently
from
3372de3 to
dabfc67
Compare
| "tests/input" | ||
| ] | ||
|
|
||
| select = [ |
There was a problem hiding this comment.
This list is all the linter extensions currently available for ruff. ATM most of these are active so the linter throws up a lot of errors; these will be fixed in an upcoming PR. Not all the linter extensions are useful or desired but I find it easier to have the list in the pyproject file rather than having to go to the ruff website and work out what you want / don't want there.
…t_stats tox setting for use in GitHub Actions.
ialarmedalien
force-pushed
the
ruff_format_and_lint
branch
from
dabfc67 to
595d95c
Compare
| """ | ||
| Transformers (aka data mappers) are used to transform objects from one class to another | ||
| using a transformation specification. | ||
| Transformers (aka data mappers) are used to transform objects from one class to another using a transformation specification. |
There was a problem hiding this comment.
I fixed various linter errors in this file whilst renaming the variable that codespell kept flagging as a typo.
|
|
||
| def map_object(self, obj: OBJECT_TYPE, source_type: str = None, **kwargs) -> OBJECT_TYPE: | ||
| def map_object( | ||
| self, obj: OBJECT_TYPE, source_type: Optional[str] = None, **kwargs: dict[str, Any] |
There was a problem hiding this comment.
add typing to kwargs
|
|
||
| def map_database( | ||
| self, source_database: Any, target_database: Optional[Any] = None, **kwargs | ||
| self, source_database: Any, target_database: Optional[Any] = None, **kwargs: dict[str, Any] |
| def load_source_schema(self, path: Union[str, Path, dict]) -> None: | ||
| """ | ||
| Sets source_schemaview from a schema path. | ||
| Set source_schemaview from a schema path. |
There was a problem hiding this comment.
According to pydocstyle, function/method documentation should be in the imperative voice.
| for ancestor in ancmap.values(): | ||
| for k, v in ancestor.__dict__.items(): |
There was a problem hiding this comment.
codespell was convinced this should be and ==> change it to ancestor to shut codespell up.
There was a problem hiding this comment.
I have made the same changes myself in other linkml repos :D
There was a problem hiding this comment.
I suppose at least it does enforce good variable naming... even if it is a bit of a nuisance!
| elif curr_v is None: | ||
| setattr(cd, k, v) |
There was a problem hiding this comment.
else:
if ...is equivalent to
elif ...
| "boolean": bool, | ||
| } | ||
| cls = cmap.get(target_range, None) | ||
| cls = cmap.get(target_range) |
There was a problem hiding this comment.
obj.get(...) returns None by default
| dev = [ | ||
| "deepdiff<7.0.0,>=6.7.1", | ||
| "deptry<1.0.0,>=0.21.1", | ||
| "jupyter<2.0.0,>=1.0.0", | ||
| "linkml>=1.7.0", | ||
| "mkdocs-mermaid2-plugin<1.0.0,>=0.6.0", | ||
| "mkdocs-windmill", | ||
| "mkdocstrings[crystal,python]", | ||
| "mknotebooks<1.0.0,>=0.8.0", | ||
| "pytest>=7.3.1", | ||
| "ruff>=0.11.2", | ||
| "tox>=4.24.2", | ||
| ] |
There was a problem hiding this comment.
I'm curious about this dev group. As I understand it, the project.optional-dependencies section is used to define package extras. So there's two implications here:
- A downstream client could theoretically do
pip install linkml-map[dev]. There's probably no valid reason for them to do that, but it's possible and maybe could cause confusion? - A developer of this package needs to remember to run
poetry install -E dev(or--all-extras) to get the development dependencies.
uv supports the dependency-groups section for development dependencies. However Poetry does not yet. They still recommend using a tool.poetry.group.<group> section.
With all that said, what do you think about moving these dependencies back to tool.poetry.group.dev.dependencies for now? I know that hampers having this be a Poetry- and uv-compatible project file, but it keeps us inline with current recommended practices. And it seems like it would be an easy switch later when Poetry supports dependency-groups. Let me know what you think.
There was a problem hiding this comment.
I'm planning to switch this repo to using uv in the next PR. Since this is a relatively low traffic repo (vs linkml-runtime or linkml), I think it should be safe just to switch. @sierra-moxon does that sound OK to you?
I can easily either double up the requirements sections (so there are temporarily both dependency-groups and tool.poetry.group.<group> sections) or comment out the new-fangled dependency-groups and restore the poetry section headers in the meantime.
There was a problem hiding this comment.
I'm planning to switch this repo to using uv in the next PR.
Ah okay. In that case, if this is just a stepping stone and then next PR will convert to using dependency-groups in short order then I'm less concerned.
4 participants
Switching from black, isort, and flake8 to ruff.
First commit runs the ruff formatter over the files with a few minor edits to fix some flake-y issues.
Second, third, fourth, and fifth commits update the infrastructure files.
Commit six fixes linter errors in
transformer.pyand updates a variable name to stop codespell complaining.TODO (these will go in upcoming PRs -- this PR is already too large):