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

QNTM-5415 Expose ToJson() and OpenJsonFileFromPath() in DynamoCore as Public methods #9161

Closed
wants to merge 5 commits into from

Conversation

saintentropy
Copy link
Contributor

@saintentropy saintentropy commented Oct 9, 2018

Purpose

The purpose of this PR is to expose methods related to Json as public methods to allow interactions with the Dynamo API's that do not require interactions with the file system. The PR does the following specifically.

  • Changes OpenJsonFileFromPath() from private to public method and allows the API user to open a Model with only the json string. This change also modifies the method parameters...
    previous: (string fileContents, string filePath, bool forceManualExecutionMode)
    new: (string fileContents, bool forceManualExecutionMode, string filePath = null)
    The filepath is now marked as optional as it is only utilized to provide a path to scan for .dyf files.

  • Change the ToJson() methods on WorkspaceModel and WorkspaceViewModel to public. This allows access to the serialized representation of this data. Previously only the JsonConverter's were public.

  • Create an API method on the DynamoViewModel called GetCurrentWorkspaceModelAndViewJson() to get the combined Json of the WorkspaceModel and WorkspaceViewModel as would be saved to the file system as .DYN or .DYF file. Currently we don't have a runtime object that represents this combined data set where a traditional ToJson() method would more logically apply. I located this method in the DynamoViewModel so that it is adjacent to current API's for Save() and `SaveAs() that relate to the same combined json data model.

TODO

  • Discuss naming :-)
  • Add tests
  • Pass CI

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @QilongTang

FYIs

@sm6srw

@QilongTang
Copy link
Contributor

@saintentropy Understand the need of exposing the method to get the whole Json, but can you talk more about the use case of exposing toJson() on both model and ViewModel?

@saintentropy
Copy link
Contributor Author

@QilongTang There are some cases where the user will only have a WorkspaceModel and will still need access to the json. This also gives the developer opportunity to get access to the serialized data for there own purposes (ie alternate views of the graph data, etc)

@QilongTang
Copy link
Contributor

@saintentropy Understood, originally I thought we only need to expose API for the whole json export, but I see no harm giving that flexibity to users. The only hope I have currently is not binding the export function to DynamoViewModel and maybe move it to WorkspaceView. Because this implementation could be less flexible when we introduce multiple workspace running again in the future, and user should still be able to get json of each workspaces without switching current workspace.

I am thinking of two APIs on WorkspaceView level

WorkspaceViewModel.toJson()
WorkspaceViewMode.getWholeGraphJson()

This should give us the flexibity that user getting the json of both currentWorkspace and customnodeWorkspace or any backgroundWorkspace.

My 2 cents

@saintentropy saintentropy changed the title [WIP] QNTM-5415 Expose ToJson() and OpenJsonFileFromPath() in DynamoCore as Public methods QNTM-5415 Expose ToJson() and OpenJsonFileFromPath() in DynamoCore as Public methods Oct 12, 2018
@saintentropy
Copy link
Contributor Author

@QilongTang I think you suggesting makes sense as well... I think we just need to decide which is the least confusing.

@saintentropy
Copy link
Contributor Author

@mjkkirschner do you have a strong preference if we move this functionality to the WorkspaceViewModel as @QilongTang suggests

@mjkkirschner
Copy link
Member

@saintentropy is this required for 2.0.2?

@QilongTang
Copy link
Contributor

@saintentropy Can you confirm this is still needed for Dynamo 2.1? Our code freeze will be before mid Jan so we need to put this in if it is required.

@QilongTang QilongTang added the DNM Do not merge. For PRs. label Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM Do not merge. For PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants