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

DYN-3738: enhance readyparams api #11726

Merged
merged 3 commits into from
Jun 2, 2021
Merged

Conversation

BogdanZavu
Copy link
Contributor

@BogdanZavu BogdanZavu commented Jun 2, 2021

Purpose

Enhance existing ReadyParams API so that we can have better control over graph opening and closing operations as part of an extension. We will use this in LintingViewExtension where we want to do some cleanup when a graph is closed and allow an extension developer to perform cleanup operations on their listing rules.

Declarations

Check these if you believe they are true

  • The codebase 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.
  • This PR modifies some build requirements and the readme is updated

@mjkkirschner
Copy link
Member

Hi @BogdanZavu, thank you for addressing the comments.
I wanted to clarify what I stated above a bit.

There are times we do break the API and document it like you originally had done. But this should only be done when theres no good alternative solution that does not break the API - or there is good enough reason to intentionally break an API.

For example - we broke (and probably will again) the Helix 3d graphics APIs within a minor release:

  • external types had leaked in the public Dynamo API.
  • use was very low.
  • most importantly, the experience for regular dynamo users was severely degraded in the older version of helix, and updating meant they no longer needed to install directX10.

In general I think principles like this should be used to judge API breaks.

  • What API is being broken/what is the benefit?
  • how many users will it effect negatively, how many positively?
  • can we do it some other way and keep both apis working for a bit longer?
  • Is the API popular? Is it an API we actively advertise (extensions) or just something that is public because of historical reasons.(helix)

public event Action<IWorkspaceModel> CurrentWorkspaceClearingStarted;
private void OnCurrentWorkspaceModelClearingStarted(IWorkspaceModel ws)
{
if (CurrentWorkspaceClearingStarted != null)
Copy link
Member

Choose a reason for hiding this comment

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

in newer c# versions we can now do this:
CurrentWorkspaceClearingStarted?.Invoke(ws);

@BogdanZavu
Copy link
Contributor Author

I was thinking of this more as a fix rather than an API change. Whoever subscribes to the clearing event most likely will get the workspace from somewhere else.

@BogdanZavu BogdanZavu merged commit 1421ad4 into DynamoDS:master Jun 2, 2021
@BogdanZavu BogdanZavu deleted the DYN-3738 branch June 2, 2021 17:45
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.

2 participants