Clean up Workflow methods; update naming for Workflow/edit endpoint#49
Clean up Workflow methods; update naming for Workflow/edit endpoint#49
Conversation
Move all back-end info to 'pyworkflow' key of JSON object. Update 'filename' accessor on Workflow object.
Adds exception handling if any info is missing and combines `from_session`, `from_file`, and `from_request` into a single `from_json` method
reelmatt
requested review from
cesaragv,
cesarnda,
diegostruk,
matthew-t-smith and
reddigari
reddigari
left a comment
reddigari
left a comment
There was a problem hiding this comment.
Awesome work, especially before whatever code review stuff we have to do!
Only concern here is regarding the use of the DiagramModel UUID as a workflow identifier. When we clear the front-end, we delete all the nodes and edges in the model, but don't create a new one. It's due to how we define the model and engine as attributes of the Workspace component (which is bad React practice, and my fault). Basically the component is not re-rendered on clear, so the model and engine instances keep their IDs. If a user were to clear their diagram and build another, it would have the same ID and probably overwrite files on the server (if the model ID is used in any file names).
We can ignore this for now and make a TODO somewhere, or I can look into a more thorough diagram-clearing handler. It will likely involve refactoring the model and engine into a class that's passed in as a prop to the Workspace component.
| return nx.readwrite.json_graph.node_link_graph(json_data) | ||
|
|
||
| @staticmethod | ||
| def generate_file_name(workflow, node_id): |
There was a problem hiding this comment.
Same here re. static method. In fact, this could probably be an instance method on the Node class. But maybe not worth it depending how many places call it (and their callers).
There was a problem hiding this comment.
The method is only used once, in store_node_data(), and I thought about removing it completely. Once generated and used for writing data, they can be looked up by accessing the data attribute on a Node, which I think is all we need (basically your idea of an instance method on the Node class?).
If there is a reason to keep this, I think it makes more sense in the Workflow class since the file name includes the Workflow name.
|
Good note re: UUID/naming. I moved the comment I was writing here to #51 as a way of making it a TODO. |
diegostruk
left a comment
diegostruk
left a comment
There was a problem hiding this comment.
Great refactoring here! I added a small comment re: making a utils class to add all file handling concerns there, would like to hear you opinion on that.
pyworkflow/pyworkflow/workflow.py
Outdated
| def path(self, file_name): | ||
| return os.path.join(self.root_dir, file_name) | ||
| @staticmethod | ||
| def path(workflow, file_name): |
There was a problem hiding this comment.
If we make this method static, should we create a utils class concerned with all the file handling? Maybe we can add also a method for determining the root_dir value for the workflow which can handle the logic in lines 21 to 29?
There was a problem hiding this comment.
I went with making path() and instance method because it seemed cleaner to me. I really like your idea about adding a utils class, and move the root_dir logic there.
retrieve_node_data and store_node_data might work better as Util methods too, but for now I'm leaving them as static within Workflow as we work more on passing data between nodes, but I think this is something we might revisit soon.
|
Changes since PR:
I think it makes sense to merge this now, and if we want to revisit these points later on, it should be easy enough to do. |
4 participants
This cleans up
pyworkflow/workflow.pyby removing unused methods, and condensing others. Namely, there wasfrom_session,from_file, andfrom_requestmethods that all read in a Workflow in mostly the same way. Now, there is onefrom_jsonmethod that can load a Workflow from a file (via the 'open' endpoint) and from session (via the middleware).Workflow attributes have been simplified to:
model.options.idpassed in from the front-end). An 'edit_workflow' endpoint has also been added to make this a more user-friendly name.pyworkflowto increase modularity of the package.Workflow.path()now handles creating valid path names for reading/saving files.These changes should not affect the existing functionality, at least for tests that contain all the required info. One area I wasn't able to test that might be affected is cases where there is missing info. The
from_jsonandto_session_dictmethods for reading/writing the Workflow now assume valid data is provided and will raise a WorkflowException if it's not there. Previously, in some situations it would assignNoneto the graph if a graph was not present, but that's not really the behavior we want (I think). Something to check/look out for.