-
Notifications
You must be signed in to change notification settings - Fork 3
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
Library Usage #118
Library Usage #118
Conversation
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.
There are at least three can't-pass-as-is issues. It looks like a few parts of this PR can be filed on their own while we work those out.
This patch partially implements a small piece of PR 118. References: * #118 Requested-by: kchason <[email protected]> Signed-off-by: Alex Nelson <[email protected]>
AJN: This is a partial application of @kchason 's work in PR 118, and is being pulled into its own patch series to focus review. References: * #118 Signed-off-by: Alex Nelson <[email protected]>
No effects were observed on Make-managed files. Signed-off-by: Alex Nelson <[email protected]>
It seems to me like `--disallow-untyped-defs`, enabled in `mypy --strict`, should have flagged this as an error. However, from documentation on `no-untyped-def`, `mypy` only requires `__init__(...) -> None` when there is any argument aside from the first `self`. This patch follows the parenthetical recommendation from PEP 484 that `-> None` be given anyways. References: * https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-disallow-untyped-defs * https://mypy.readthedocs.io/en/stable/error_code_list2.html#check-that-every-function-has-an-annotation-no-untyped-def * https://peps.python.org/pep-0484/#the-meaning-of-annotations Signed-off-by: Alex Nelson <[email protected]>
… type Signed-off-by: Alex Nelson <[email protected]>
case_utils/case_validate/__init__.py
Outdated
validate_result = pyshacl.validate( | ||
validate_result: Tuple[ | ||
bool, Union[Exception, bytes, str, rdflib.Graph], str | ||
] = pyshacl.validate( |
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 think this call needs to be replaced with the validate()
method this PR is adding to this file, but only after logistics related to #123 are settled.
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 believe this is addressed in 15f00c9
.
Signed-off-by: Alex Nelson <[email protected]>
…n "none" This is a continuation of PR 123. References: * #123 Signed-off-by: Alex Nelson <[email protected]>
case_utils/case_validate/__init__.py
Outdated
_logger = logging.getLogger(os.path.basename(__file__)) | ||
|
||
|
||
class NonExistentCDOConceptWarning(UserWarning): | ||
def validate( | ||
input_file: str, |
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.
For our awareness, this is a narrower argument than the first argument of pyshacl.validate
; here's today's definition:
https://github.com/RDFLib/pySHACL/blob/v0.23.0/pyshacl/validate.py#L369-L370
pyshacl.validate
's first argument seems to permit a string to be a file path or URL, OR a full string dump of a graph. See these lines for heuristics in pyshacl.rdfutil.load.load_from_graph
:
https://github.com/RDFLib/pySHACL/blob/v0.23.0/pyshacl/rdfutil/load.py#L222-L227
Should we implement "str
means path" now, or just adopt the load_from_graph
usage now from these lines:
https://github.com/RDFLib/pySHACL/blob/v0.23.0/pyshacl/validate.py#L424-L428
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.
Welp, I found an issue pushing us towards expanding input_file
from str
. case_validate
is written to take multiple input files as data graphs (as well as multiple input files as ontology graphs). I believe this behavior should be preserved, because otherwise a user that needs to read two data graphs at once needs to do some intermediary graph compilation before calling case_validate
.
So, I think the first argument needs to become at least either Union[str, List[str]]
or Union[str, Graph]
. The current code path from the CLI entry point I think favors Union[str, List[str]]
.
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 believe this is addressed in 15f00c9
.
…idate This patch separates implementation points between functionality distinct to `case_utils.validate` and `pyshacl.validate`. The `allow_warnings` and `inference` parameters provide CASE-specific documentation as an augmentation to `pyshacl.validate`'s documentation, but otherwise other documentation on `pyshacl.validate`'s keyword arguments is delegated to their upstream function. This patch removes some hardcoded parameter values in `pyshacl.validate`, letting the `case_validate` CLI or caller provide any runtime-requested values. Also, without functional impact, this patch sorts keyword parameters alphabetically. Signed-off-by: Alex Nelson <[email protected]>
Signed-off-by: Alex Nelson <[email protected]>
Signed-off-by: Alex Nelson <[email protected]>
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'm fine with this merging once CI passes. Also open to discussing any of the last revisions I made.
Example usage of the programmatic usage of the CASE utilities.