Skip to content

Ruff format and lint #52

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.

Sign up for GitHub

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 6 commits into from
Apr 14, 2025
Merged

Ruff format and lint #52

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
758bc40
Run ruff format over codebase
ialarmedalien Apr 2, 2025
47c963d
Updating infrastructure files
ialarmedalien Apr 2, 2025
9088ac8
updating pyproject to move redundant package calls
ialarmedalien Apr 2, 2025
d96a768
Updating poetry lock file
ialarmedalien Apr 2, 2025
4004fad
Run codespell, linter, and tests on all pull requests in an open, non…
ialarmedalien Apr 3, 2025
595d95c
fix `anc` in transformer.py to stop codespell complaining. Create lin…
ialarmedalien Apr 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ jobs:
run: |
pip install tox

- name: Check code quality with flake8
run: tox -e lint
- name: Check code quality with ruff
run: tox -e lint_stats

test:
runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

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

Below, also inject poetry dynamic versioning into pipx-installed poetry:

  - name: Install Poetry
      run: |
        pipx install poetry
        pipx inject poetry poetry-dynamic-versioning

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Versioning isn't really needed to run the tests, is it?

Copy link
Member

Choose a reason for hiding this comment

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

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".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

strategy:
matrix:
python-version: [ '3.9', '3.10', '3.11', '3.12' ]
python-version: [ '3.9', '3.10', '3.11', '3.12', '3.13' ]

steps:
- uses: actions/checkout@v4
Expand Down
24 changes: 18 additions & 6 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ authors = [
{name = "cmungall", email = "cjm@berkeleybop.org"},
]
requires-python = "<=3.13,>=3.9"
dynamic = [ "version" ]

dependencies = [
"asteval<1,>=0",
Expand Down Expand Up @@ -47,6 +48,13 @@ dev = [
[project.scripts]
linkml-map = "linkml_map.cli.cli:main"

[tool.poetry]
requires-poetry = ">=2.0"
version = "0.0.0"

[tool.poetry.requires-plugins]
poetry-dynamic-versioning = { version = ">=1.0.0,<2.0.0", extras = ["plugin"] }

[tool.poetry-dynamic-versioning]
enable = true
vcs = "git"
Expand Down Expand Up @@ -74,12 +82,14 @@ target-version = "py312"

[tool.ruff.format]
exclude = [
"docs/",
"src/linkml_map/datamodel",
"tests/input"
]

[tool.ruff.lint]
exclude = [
"docs/",
"src/linkml_map/datamodel",
"tests/input"
]
Expand Down Expand Up @@ -130,16 +140,16 @@ select = [
"TCH", # flake8-type-checking
"INT", # flake8-gettext
"ARG", # flake8-unused-arguments
"PTH", # flake8-use-pathlib
"TD", # flake8-todos
"FIX", # flake8-fixme
# "PTH", # flake8-use-pathlib
# "TD", # flake8-todos
# "FIX", # flake8-fixme
"ERA", # eradicate
# "PD", # pandas-vet
"PD", # pandas-vet
"PGH", # pygrep-hooks
"PL", # Pylint
"TRY", # tryceratops
"FLY", # flynt
# "NPY", # NumPy-specific rules
"NPY", # NumPy-specific rules
"AIR", # Airflow
"PERF", # Perflint
"FURB", # refurb
Expand Down Expand Up @@ -167,7 +177,9 @@ ignore = [
# E731: lambda-assignment - needs code change
"E731",
# B024: abstract-base-class-without-abstract-method
"B024"
"B024",
# T201: print
"T201"
]

[tool.ruff.lint.per-file-ignores]
Expand Down
47 changes: 23 additions & 24 deletions src/linkml_map/transformer/transformer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
"""
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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed various linter errors in this file whilst renaming the variable that codespell kept flagging as a typo.

"""

import logging
from abc import ABC
from copy import deepcopy
from dataclasses import dataclass, field
from pathlib import Path
from typing import Any, Dict, Optional, Union
from typing import Any, Optional, Union

import yaml
from curies import Converter
Expand All @@ -30,7 +29,7 @@
logger = logging.getLogger(__name__)


OBJECT_TYPE = Union[Dict[str, Any], BaseModel, YAMLRoot]
OBJECT_TYPE = Union[dict[str, Any], BaseModel, YAMLRoot]
"""An object can be a plain python dict, a pydantic object, or a linkml YAMLRoot"""


Expand Down Expand Up @@ -63,7 +62,9 @@ class Transformer(ABC):

_curie_converter: Converter = None

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]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

add typing to kwargs

) -> OBJECT_TYPE:
"""
Transform source object into an instance of the target class.

Expand All @@ -74,7 +75,7 @@ def map_object(self, obj: OBJECT_TYPE, source_type: str = None, **kwargs) -> OBJ
raise NotImplementedError

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]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

add typing

) -> OBJECT_TYPE:
"""
Transform source resource.
Expand All @@ -86,25 +87,23 @@ def map_database(
"""
raise NotImplementedError

def load_source_schema(self, path: Union[str, Path, dict]):
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.
Copy link
Collaborator Author

@ialarmedalien ialarmedalien Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

According to pydocstyle, function/method documentation should be in the imperative voice.


:param path:
:return:
"""
if isinstance(path, Path):
path = str(path)
self.source_schemaview = SchemaView(path)

def load_transformer_specification(self, path: Union[str, Path]):
def load_transformer_specification(self, path: Union[str, Path]) -> None:
"""
Sets specification from a schema path.
Set specification from a schema path.

:param path:
:return:
"""
# self.specification = yaml_loader.load(str(path), TransformationSpecification)
with open(path) as f:
obj = yaml.safe_load(f)
# necessary to expand first
Expand All @@ -115,9 +114,9 @@ def load_transformer_specification(self, path: Union[str, Path]):
obj = normalizer.normalize(obj)
self.specification = TransformationSpecification(**obj)

def create_transformer_specification(self, obj: Dict[str, Any]):
def create_transformer_specification(self, obj: dict[str, Any]) -> None:
"""
Creates specification from a dict.
Create specification from a dict.

TODO: this will no longer be necessary when pydantic supports inlined as dict

Expand Down Expand Up @@ -157,22 +156,22 @@ def _get_class_derivation(self, target_class_name: str) -> ClassDerivation:
ancmap = self._class_derivation_ancestors(cd)
if ancmap:
cd = deepcopy(cd)
for anc in ancmap.values():
for k, v in anc.__dict__.items():
for ancestor in ancmap.values():
for k, v in ancestor.__dict__.items():
Comment on lines +159 to +160
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

codespell was convinced this should be and ==> change it to ancestor to shut codespell up.

Copy link
Member

Choose a reason for hiding this comment

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

I have made the same changes myself in other linkml repos :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose at least it does enforce good variable naming... even if it is a bit of a nuisance!

if v is not None and v != []:
curr_v = getattr(cd, k, None)
if isinstance(curr_v, list):
curr_v.extend(v)
elif isinstance(curr_v, dict):
curr_v.update({**v, **curr_v})
else:
if curr_v is None:
setattr(cd, k, v)
elif curr_v is None:
setattr(cd, k, v)
Comment on lines +167 to +168
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  else:
    if ...

is equivalent to

  elif ...

return cd

def _class_derivation_ancestors(self, cd: ClassDerivation) -> Dict[str, ClassDerivation]:
def _class_derivation_ancestors(self, cd: ClassDerivation) -> dict[str, ClassDerivation]:
"""
Returns a map of all class derivations that are ancestors of the given class derivation.
Return a map of all class derivations that are ancestors of the given class derivation.

:param cd:
:return:
"""
Expand All @@ -199,7 +198,7 @@ def _get_enum_derivation(self, target_enum_name: str) -> EnumDerivation:

def _is_coerce_to_multivalued(
self, slot_derivation: SlotDerivation, class_derivation: ClassDerivation
):
) -> bool:
cast_as = slot_derivation.cast_collection_as
if cast_as and cast_as in [
CollectionType.MultiValued,
Expand All @@ -218,7 +217,7 @@ def _is_coerce_to_multivalued(

def _is_coerce_to_singlevalued(
self, slot_derivation: SlotDerivation, class_derivation: ClassDerivation
):
) -> bool:
cast_as = slot_derivation.cast_collection_as
if cast_as and cast_as == CollectionType(CollectionType.SingleValued):
return True
Expand All @@ -244,7 +243,7 @@ def _coerce_datatype(self, v: Any, target_range: Optional[str]) -> Any:
"string": str,
"boolean": bool,
}
cls = cmap.get(target_range, None)
cls = cmap.get(target_range)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

obj.get(...) returns None by default

if not cls:
logger.warning(f"Unknown target range {target_range}")
return v
Expand Down
11 changes: 10 additions & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ isolated_build = true
envlist =
format
lint
lint_stats

[testenv:format]
deps =
Expand All @@ -23,4 +24,12 @@ deps =
ruff
commands =
ruff check src/ tests/
description = Run the ruff tool with numerous plugins (bandit, docstrings, import order, pep8 naming).
description = Run ruff code linter.

[testenv:lint_stats]
skip_install = true
deps =
ruff
commands =
ruff check src/ tests/ --statistics --exit-zero
description = Run ruff code linter and output statistics about lint errors. Ruff will exit with a non-zero exit code if it terminates abnormally, but lint errors will not cause failure.