Skip to content
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

Enable debug option for pyschacl validation #281

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

crisely09
Copy link
Contributor

@crisely09 crisely09 commented Nov 17, 2022

In the effort to solve #280 this option let's us use the debug version of the pyschacl validator.

@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

Codecov Report

Merging #281 (3e3fe1a) into master (a1f7242) will decrease coverage by 0.23%.
The diff coverage is 48.68%.

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
- Coverage   72.62%   72.40%   -0.23%     
==========================================
  Files          90       90              
  Lines        5761     5787      +26     
==========================================
+ Hits         4184     4190       +6     
- Misses       1577     1597      +20     
Flag Coverage Δ
unittests 72.40% <48.68%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
kgforge/specializations/stores/bluebrain_nexus.py 26.53% <18.18%> (-0.49%) ⬇️
kgforge/core/forge.py 64.51% <33.33%> (-0.87%) ⬇️
...gforge/specializations/models/rdf/store_service.py 20.56% <50.00%> (ø)
kgforge/core/archetypes/model.py 57.14% <100.00%> (ø)
kgforge/core/archetypes/store.py 58.98% <100.00%> (ø)
kgforge/specializations/models/demo_model.py 67.05% <100.00%> (ø)
...ge/specializations/models/rdf/directory_service.py 92.15% <100.00%> (ø)
kgforge/specializations/models/rdf/service.py 87.34% <100.00%> (ø)
kgforge/specializations/models/rdf_model.py 82.55% <100.00%> (ø)
tests/specializations/models/test_demo_model.py 100.00% <100.00%> (ø)
... and 1 more

@crisely09 crisely09 requested a review from MFSY November 17, 2022 13:56
run(
self._register_one,
None,
data,
required_synchronized=False,
execute_actions=True,
catch_exceptions=catch_exceptions,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would do:
catch_exceptions= not debug

@crisely09
Copy link
Contributor Author

I haven't implemented yet the option to delete distributions and deprecate files when registration fails. I would rather put that in a separate PR.

messages.append(str(the_details))
else:
messages.append(str(result))
return ". ".join(messages)

Choose a reason for hiding this comment

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

Is it an absolute must to join these? Or, better yet, does the result dictionaries have to be converted to str?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a choice, would you like to have the full schacl error result? Maybe we can give it back when the debug option is true. However, if the intention is to make a nicer Error log, we can add it here, I don't know, what do you think?

Copy link

Choose a reason for hiding this comment

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

No, no. Not the full shacl output, absolutely not. It's just for flexibility of further handling the output.
For example, if the dict you gather above is kept as a dict, it would allow to catch and handle the output on user side like:

try:  # this would happen in nexus-forge
    reason = "Flux Capacitor is broken"
    troubleshooting = {"attention": "Disconnect capacitor drive before opening", "safety": "Shield eyes from light", "actions": "do magic"}
    raise ValueError(reason, troubleshooting)
except ValueError as e:  # <-- error-handling on user side
    print(f"ERROR: {e.args[0]}")
    print("\nTroubleshooting Tips:")
    for k,v in e.args[1].items():
        print(f"\t{k.ljust(10)}: {v}")

OUTPUT:

ERROR: Flux Capacitor is broken

Troubleshooting Tips:
        attention : Disconnect capacitor drive before opening
        safety    : Shield eyes from light
        actions   : do magic

or we can just leave it as it is:

raise ValueError(reason, troubleshooting)

OUTPUT:

# <traceback>
ValueError: ('Flux Capacitor is broken', {'attention': 'Disconnect capacitor drive before opening', 'safety': 'Shield eyes from light', 'actions': 'do magic'})

Choose a reason for hiding this comment

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

On nexus-forge side, you would still able to format the messages any way you want in execution.py. What do you think?

Copy link
Contributor Author

@crisely09 crisely09 Nov 25, 2022

Choose a reason for hiding this comment

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

Ah ok ok, I understand now your point, yes, I like it, I will make some changes, still not decided on if returning the dictionary, but already improved the message.

@joni-herttuainen
Copy link

I was testing the newest changes, and we're discussing about it in NSETM-2067.

Instead of opening a new ticket, I'll relay a comment from @jdcourcol, before this is pushed to master:

This is not great. "RegistrationError" does not bring any information when you are "Registering". The client knows that he is registering. It should be a "ValidationError" exception instead. And then having the information as text is unusable for programmatic access.

Copy link
Contributor

@ssssarah ssssarah left a comment

Choose a reason for hiding this comment

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

I have yet to run things to see what happens

kgforge/specializations/models/rdf_model.py Outdated Show resolved Hide resolved
kgforge/core/archetypes/store.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants