SchemaView: format, lint autofix, add typing + docs#389
SchemaView: format, lint autofix, add typing + docs#389kevinschaper merged 3 commits intolinkml:mainfrom
Conversation
…n * import Run ruff format on schemaview and schemaview tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
=======================================
Coverage 63.79% 63.79%
=======================================
Files 63 63
Lines 8946 8938 -8
Branches 2587 2584 -3
=======================================
- Hits 5707 5702 -5
+ Misses 2633 2629 -4
- Partials 606 607 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| from collections.abc import Mapping, Iterator | ||
|
|
||
| from linkml_runtime import SchemaView | ||
| from linkml_runtime.utils.schemaview import SchemaView |
There was a problem hiding this comment.
use full path for import (this was mostly to make it easier for me to find places where weird stuff was being imported from SchemaView)
| @@ -1,37 +1,68 @@ | |||
| """SchemaView, a virtual schema layered on top of a schema plus its import closure.""" | |||
|
|
|||
| from __future__ import annotations | |||
There was a problem hiding this comment.
allows nicer ways of expressing typing (i.e. X | Y, etc.)
| if ordered_by in (OrderedBy.LEXICAL, OrderedBy.LEXICAL.value): | ||
| return self._order_lexically(elements) | ||
| elif ordered_by in (OrderedBy.RANK, OrderedBy.RANK.value): | ||
| if ordered_by in (OrderedBy.RANK, OrderedBy.RANK.value): |
There was a problem hiding this comment.
no need for else or elif after a return
| candidate = clist[i] | ||
| can_add = False | ||
| if candidate.is_a is None: | ||
| if candidate.is_a is None or candidate.is_a in [p.name for p in slist]: |
There was a problem hiding this comment.
collapse / combine if conditions
| classes = copy(self._get_dict(CLASSES, imports)) | ||
| classes = self.ordered(classes, ordered_by=ordered_by) | ||
| return classes | ||
| return self.ordered(classes, ordered_by=ordered_by) |
There was a problem hiding this comment.
return directly, no need to assign to a variable
|
|
||
| slots = self.ordered(slots, ordered_by=ordered_by) | ||
| return slots | ||
| return self.ordered(slots, ordered_by=ordered_by) |
There was a problem hiding this comment.
return directly
| :return: all enums in schema view | ||
| """ | ||
| return self._get_dict(ENUMS, imports) | ||
| return self.all_enums(imports) |
There was a problem hiding this comment.
redirect to the non-deprecated function
| all_subsets = self.all_subsets(imports=imports) | ||
| # {**a,**b} syntax merges dictionary a and b into a single dictionary, removing duplicates. | ||
| return {**all_classes, **all_slots, **all_enums, **all_types, **all_subsets} | ||
| return self.all_elements(imports) |
There was a problem hiding this comment.
redirect to non-deprecated function
| return _closure(lambda x: self.class_parents(x, imports=imports, mixins=mixins, is_a=is_a), | ||
| class_name, | ||
| reflexive=reflexive, depth_first=depth_first) | ||
| return _closure( |
| if not is_empty(v2): | ||
| v = v2 | ||
| logger.debug(f'{v} takes precedence over {v2} for {induced_slot.name}.{metaslot_name}') | ||
| elif metaslot_name in COMBINE: |
There was a problem hiding this comment.
else + if => elif
| :return: | ||
| """ | ||
| self.schema.subsets[subset.name] = type | ||
| self.schema.subsets[subset.name] = subset |
| :return: | ||
| """ | ||
| self.schema.types[type.name] = type | ||
| self.schema.types[type_def.name] = type_def |
There was a problem hiding this comment.
type is a reserved word in python
| schema_path = os.path.join(INPUT_DIR, "schemaview_is_inlined.yaml") | ||
| sv = SchemaView(schema_path) | ||
| cases = [ | ||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
use parametrize instead of iterating through a list
|
|
||
| @pytest.fixture | ||
| def view(): | ||
| def schema_view_with_imports() -> SchemaView: |
| @pytest.fixture(scope="session") | ||
| def schema_view_attributes() -> SchemaView: |
There was a problem hiding this comment.
add a fixture for this since it's used in several tests
| """ | ||
| view = SchemaView(os.path.join(INPUT_DIR, "attribute_edge_cases.yaml")) | ||
| expected = [ | ||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
parametric instead of iterating through a list
|
@ialarmedalien do I understand it correctly? The conversations that you've opened in this PR are mostly justifying some of the changes, right? So a reviewer that holds those changes as meaningful, can simply comment "makes sense" or so or "thumb-up" your comment and close those conversations, right? |
|
@ialarmedalien I'm trying to run "ruff" myself just to be sure that I don't look at changes directly applied by the tool and I'm failing. I'm getting an error message. [Edit]: I've seen that Doing so I've realized that we are not specifying I've created this PR to your branch to add it. It might be a good idea to have separate commits to add whatever other ruff linters you think would be meaningful for the project. |
|
@ialarmedalien I've realized that you are running I'm trying to review this PR, but I'm having difficulties to segregate those changes introduced by [Edit]: I've just found out that you are running These are the commands to reproduce my environment: $ git clone --quiet https://github.com/linkml/linkml-runtime
$ cd linkml-runtime
$ git remote add silvanoc https://github.com/Silvanoc/linkml-runtime
$ git fetch --quiet silvanoc
$ git cherry-pick silvanoc/add-ruff-as-dev-dependency~1
[main a2a0a4e] chore: add ruff to dev dependencies
Date: Wed May 7 12:41:47 2025 +0200
2 files changed, 31 insertions(+), 1 deletion(-)
$ git cherry-pick silvanoc/add-ruff-as-dev-dependency
[main b376d8e] chore(lint): add import sorting
Date: Wed May 7 13:15:32 2025 +0200
1 file changed, 2 insertions(+), 1 deletion(-)
$ poetry install --quiet --only=devIf I let $ poetry run ruff check --fix linkml_runtime/utils/schemaview.py --config pyproject.toml
Found 2 errors (2 fixed, 0 remaining).
$ git diff
diff --git i/linkml_runtime/utils/schemaview.py w/linkml_runtime/utils/schemaview.py
index 1be70a3..3c8145e 100644
--- i/linkml_runtime/utils/schemaview.py
+++ w/linkml_runtime/utils/schemaview.py
@@ -1,25 +1,25 @@
+import collections
+import logging
import os
import sys
import uuid
-import logging
-import collections
-from functools import lru_cache
-from copy import copy, deepcopy
+import warnings
from collections import defaultdict, deque
+from collections.abc import Mapping
+from copy import copy, deepcopy
+from enum import Enum
+from functools import lru_cache
from pathlib import Path, PurePath
from typing import Optional, TypeVar
-from collections.abc import Mapping
-import warnings
-from urllib.parse import urlparse
-from linkml_runtime.utils.namespaces import Namespaces
from deprecated.classic import deprecated
-from linkml_runtime.utils.context_utils import parse_import_map, map_import
-from linkml_runtime.utils.formatutils import camelcase, is_empty, sfx, underscore
-from linkml_runtime.utils.pattern import PatternResolver
-from linkml_runtime.linkml_model.meta import *
+
from linkml_runtime.exceptions import OrderingError
-from enum import Enum
+from linkml_runtime.linkml_model.meta import *
+from linkml_runtime.utils.context_utils import map_import, parse_import_map
+from linkml_runtime.utils.formatutils import camelcase, is_empty, sfx, underscore
+from linkml_runtime.utils.namespaces import Namespaces
+from linkml_runtime.utils.pattern import PatternResolver
logger = logging.getLogger(__name__)The same can be done for the tests file and also with |
The comments are to explain what I did, yes, to make life easier for the reviewer. There's no need to respond unless you particularly want to! 🙂 |
|
IMO the whole linting, ruff, tox,... set-up in both If looking at Perhaps doing it "right" here first and using it as a pattern for the |
|
@Silvanoc re: ruff linting: I think it would be better if you made a direct PR to the linkml-runtime repo to update ruff and add it to the dev dependencies so that it gets the attention of the linkml-runtime maintainers. The main reason I didn't add my config into this PR is that I have pretty much all the linters enabled locally, which I would not necessarily suggest for a repo like This is my local config: I just ran I would love to format and lint auto-fix all the files in this repo, but there is an existing PR (#347) that does this and I didn't want to redo work that has already been done. |
I agree completely! I did some work on the |
The problem without the ruff configuration (thanks for providing it) is that I wanted to look at this PR and I was not capable of taking the chaff (whatever changes Anyway, you can simply ignore/close my PR on your branch.
True! I saw it in the past and forgot it! I'll have a look at it. |
|
@Silvanoc Apologies for unintentionally giving you a load of detective work to do in figuring out my ruff config! I would have included it as a comment on the PR if I had known. If you look at the last commit of the PR, it contains the edits that I made manually or via VSCode's ruff plugin code fixing feature. Hopefully that will be a little easier to review... |
No worries.
I'm proposing here some changes to streamline linting and formatting. That way we have a clear definition of what we expect, we also enforce it and we enable developers to have it locally. |
ialarmedalien
force-pushed
the
schemaview_tidy
branch
from
4c7db80 to
c7e1c3a
Compare
kevinschaper
left a comment
kevinschaper
left a comment
There was a problem hiding this comment.
thank you for the big cleanup! (and especially catching that add_subset bug!)
|
Hmm, I'm not sure this PR was expected to get merged before #347. @ialarmedalien can you please confirm? |
3 participants
upstream_repo: dalito/linkml
upstream_branch: issue2578-fix-uri-in-snapshot
Note: upstream tests will fail until linkml/linkml#2648 is merged, which relies on a new release of linkml-runtime being made.
I have a minor change to make to the SchemaView package so wanted to spruce up SV and its tests first.
I ran
ruff formatandruff check --fixto fix "safe" linting errors. See my local ruff config.The changes look rather daunting, but they are all fairly simple. I recommend viewing the commits separately if you have any worries.
commit 1: run ruff format on the files. Alter schemaview.py to pull in specific classes from the
linkml_runtime.linkml_model.metainstead of*-importingcommit 2: run the ruff linter auto fixes
commit 3: add in missing typing and basic doc strings. A couple of minor fixes, which I will add comments to.
There is an existing PR that runs formatting and linting auto fixes on the whole codebase, so I have not put in any ruff infrastructure here.