-
Notifications
You must be signed in to change notification settings - Fork 29
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
Ensure that relative imports can be imported without requiring ./
in front of the import file name
#350
base: main
Are you sure you want to change the base?
Ensure that relative imports can be imported without requiring ./
in front of the import file name
#350
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,25 +108,6 @@ def is_absolute_path(path: str) -> bool: | |
drive, tail = os.path.splitdrive(norm_path) | ||
return bool(drive and tail) | ||
|
||
def _resolve_import(source_sch: str, imported_sch: str) -> str: | ||
if os.path.isabs(imported_sch): | ||
# Absolute import paths are not modified | ||
return imported_sch | ||
if urlparse(imported_sch).scheme: | ||
# File with URL schemes are not modified | ||
return imported_sch | ||
|
||
if WINDOWS: | ||
path = PurePath(os.path.normpath(PurePath(source_sch).parent / imported_sch)).as_posix() | ||
else: | ||
path = os.path.normpath(str(Path(source_sch).parent / imported_sch)) | ||
|
||
if imported_sch.startswith(".") and not path.startswith("."): | ||
# Above condition handles cases where both source schema and imported schema are relative paths: these should remain relative | ||
return f"./{path}" | ||
Comment on lines
-124
to
-126
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? It looks like this was put in to fudge test results -- that is not a good reason to keep it in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see #368 (comment) they are functionally equivalent. i mildly prefer having explicit relative path annotations, or some way of telling that these are relative paths/paths at all, but yes string conventions are a weak way of doing that compared to proper typing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with your (linked) comment about wanting the explicit paths -- when I first looked at this code, I was wondering about the possibility of using |
||
|
||
return path | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. summary of logic in this function:
==> this can be refactored and simplified |
||
|
||
|
||
@dataclass | ||
class SchemaUsage: | ||
|
@@ -319,14 +300,24 @@ def imports_closure(self, imports: bool = True, traverse: Optional[bool] = None, | |
# path, and the target import doesn't have : (as in a curie or a URI) | ||
# we prepend the relative path. This WILL make the key in the `schema_map` not | ||
# equal to the literal text specified in the importing schema, but this is | ||
# essential to sensible deduplication: eg. for | ||
# essential to sensible deduplication: e.g. for | ||
# - main.yaml (imports ./types.yaml, ./subdir/subschema.yaml) | ||
# - types.yaml | ||
# - subdir/subschema.yaml (imports ./types.yaml) | ||
# - subdir/types.yaml | ||
# we should treat the two `types.yaml` as separate schemas from the POV of the | ||
# origin schema. | ||
i = _resolve_import(sn, i) | ||
|
||
# if i is not a CURIE and sn looks like a path with at least one parent folder, | ||
# normalise i with respect to sn | ||
if "/" in sn and ":" not in i: | ||
if WINDOWS: | ||
# This cannot be simplified. os.path.normpath() must be called before .as_posix() | ||
i = PurePath( | ||
os.path.normpath(PurePath(sn).parent / i) | ||
).as_posix() | ||
else: | ||
i = os.path.normpath(str(Path(sn).parent / i)) | ||
todo.append(i) | ||
|
||
# add item to closure | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
id: four | ||
name: import_four | ||
title: Import Four | ||
description: | | ||
Import loaded by the StepChild class. | ||
imports: | ||
- linkml:types | ||
classes: | ||
Four: | ||
attributes: | ||
value: | ||
range: string | ||
ifabsent: "Four" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
id: one | ||
name: import_one | ||
title: Import One | ||
description: | | ||
Import loaded by the StepChild class. | ||
imports: | ||
- linkml:types | ||
- two | ||
classes: | ||
One: | ||
attributes: | ||
value: | ||
range: string | ||
ifabsent: "One" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
id: stepchild | ||
name: stepchild | ||
title: stepchild | ||
description: | | ||
Child class that imports files in the same directory as itself without consistently using `./` in the link notation. | ||
imports: | ||
- linkml:types | ||
- one | ||
- two | ||
- ./three | ||
classes: | ||
StepChild: | ||
attributes: | ||
value: | ||
range: string | ||
ifabsent: "StepChild" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
id: three | ||
name: import_three | ||
title: Import Three | ||
description: | | ||
Import loaded by the StepChild class. | ||
imports: | ||
- linkml:types | ||
- ./four | ||
classes: | ||
Three: | ||
attributes: | ||
value: | ||
range: string | ||
ifabsent: "Three" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
id: two | ||
name: import_two | ||
title: Import Two | ||
description: | | ||
Import loaded by the StepChild class. | ||
imports: | ||
- linkml:types | ||
classes: | ||
Two: | ||
attributes: | ||
value: | ||
range: string | ||
ifabsent: "Two" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
urlparse
does not check for URL validity, which isn't obvious if you haven't read the docs for the function. A better test here would be for:
, which should only occur in well-formed URLs and CURIEs, but not in file system paths.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also --
imported_sch
starting withfile://
andfile:///
do not resolve andhttp
/https
/ etc. will be rejected unless there's a mapping in theprefixes
section of the schema.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz add failing tests illustrating these cases and then fix :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't clear to me what the desired behaviour is here, which is the main reason why I didn't do anything. I thought that the point of the
prefixes
section was so that external resources could be referred to without using URLs - specify the URL there and the importer Does The Right Thing automatically.file://
(orfile:///
) don't work at all due to bugs in the underlyinghbreader
package. I don't know whether this is a problem throughout the codebase, but I think I would consider ditching the package and using something more standard if so.The docs/code are a bit ambiguous as the code enforces prefixes but the docs (or at least the
imports
slot range,uriorcurie
) suggest URLs are OK. The documentation page on imports only talks about CURIEs or local paths, so I don't think that URIs should be allowed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ETA: I've just realised it's a code comment that's leading me astray:
The docs are pretty clear that the field contents should be local paths or CURIEs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would assume the metamodel spec of xsd:anyURI is correct over the docs, since the metamodel is a formal specification? what makes you say the docs are clear(er than the metamodel?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://linkml.io/linkml/schemas/imports.html#imports:
(emphasis added)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The section below describes how to import external schemas (e.g. from the web) by assigning them a prefix, giving the example of the
linkml:types
schema.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, but in general i would think that a formal specification would be the source of truth, and docs are just a description of it that can be imprecise. for example it's not really clear what a "local import" is. Since i'm a language user i interpret that as meaning "local path," but the spec has no such ambiguity. the same thing that allows the prefix notation is what would allow URIs - the range is
URIOrCurie
and those curie prefixes expand into URIs.edit: asked in chat since i'm not a core maintainer or anything and don't want to lead us off base here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for asking in slack.
I should emphasise that the implementation assumes that anything curie-like is treated as a prefixed entity with an entry for that prefix in the
prefixes
section -- i.e. if you havehttp://example.com/schema
andftp://my-fave-schemas.com/path/to/file
in yourimports
, it expects there to be entries forhttp
andftp
in theprefixes
section, and throws an error if there are not.