Fds 1843 use graph #1425
Fds 1843 use graph #1425
Conversation
|
@afwillia Thanks for the PR. Please add PR description, including linking Jira issue with this PR and making sure that tests are passing. Also, I am not seeing any file changes related to tests, so please also change tests to make sure that we are testing data_model_graph_pickle as an additional parameter. |
…model_labels=display_label
|
@afwillia It looks liky you are missing tests for |
| ) -> None: | ||
| self.path_to_jsonld = path_to_jsonld | ||
|
|
||
| self.jsonld = load_json(self.path_to_jsonld) | ||
| if data_model_graph_pickle and not graph_data_model: | ||
| with open(data_model_graph_pickle, "rb") as file: | ||
| graph_data_model = pickle.load(file) |
There was a problem hiding this comment.
If I am reading this code correctly, does this accomplish anything?
This is being set on L53
self.graph_data_model = data_model_grapher.graph
On this L39 you are updating the attribute passed in if None:
graph_data_model = pickle.load(file)
But then graph_data_model is not actually being used anywhere to set anything on the AttributesExplorer class instance that was instantiated.
There was a problem hiding this comment.
Nice catch, thanks. I updated this to set self.graph_data_model on L37 and inside if not data_model_grapher on L47
| pytest.param(("", "example.model.pickle"), marks=pytest.mark.xfail), | ||
| ] | ||
| ) | ||
| def attributes_explorer(request, helpers): |
There was a problem hiding this comment.
Take a look at this doc here: https://www.draconianoverlord.com/2017/11/28/using-given/when/then-for-tests.html/
Personally, I am a huge fan of giving each test a very explicit purpose. The way I do this is clearly state a set of criteria the test is expected to following in a:
# GIVEN some initial setup (Explain what the setup is doing)
# WHEN I apply some action after the initial setup (The functions you are testing)
# THEN I expect the output/behavior to look like (Add your assertions)
I added some tests like this in: #1472 if you wanted some examples
Co-authored-by: BryanFauble <[email protected]>
…will ever be an issue
| data_model_parser = DataModelParser( | ||
| path_to_data_model=self.path_to_json_ld, | ||
| ) | ||
| if not data_model_graph_pickle: |
There was a problem hiding this comment.
Nit: Avoid doing this unless data_model_graph_pickle is a boolean.
if data_model_graph_pickle is not None
|
|
I appreciate everyone's feedback and have incorporated most of it into this PR. @andrewelamb @linglp do you think this looks ready to merge? I have not addressed @BryanFauble 's comment about creating more readable tests, but I think the coverage is there. |
schematic/manifest/generator.py
Outdated
| @@ -1677,16 +1682,25 @@ def create_manifests( | |||
| "Please check your submission and try again." | |||
| ) | |||
|
|
|||
| data_model_parser = DataModelParser(path_to_data_model=path_to_data_model) | |||
| if not graph_data_model: | |||
There was a problem hiding this comment.
You might want this to be if graph_data_model is None I'm not sure how nx.MultiDiGraph handles thruthyness.
|
Closing due to other PR |
Address FDS-1843. The purpose of this PR is to update all functions that accept a jsonld schema to also accept a pickle version of the graph. When a graph is supplied instead of a schema, the functions will skip the time-consuming step of reading the schema into a graph.