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

Tree: edit flow diagrams for readme #11602

Merged
merged 4 commits into from Aug 22, 2022

Conversation

CraigMacomber
Copy link
Contributor

No description provided.

@CraigMacomber CraigMacomber requested a review from a team as a code owner August 19, 2022 18:10
@github-actions github-actions bot added area: dds Issues related to distributed data structures base: main PRs targeted against main branch labels Aug 19, 2022
This shows editing during a transaction:

```mermaid
graph RL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this (and the one below) meant to be LR? It's a bit hard to read in the data flow sense but I can also see how RL makes sense when it comes to editing back in a pipe. I prefer LR for this though since it isn't a pipe.

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 was on purpose. I mostly wanted the components to be in roughly the same places in all the data flow diagrams, and in this one the data flows mainly in the reverse direction.

Copy link
Contributor

@jenn-le jenn-le Aug 20, 2022

Choose a reason for hiding this comment

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

Oh I see. Using subgraphs might help make this more clear if you keep some consistent groups of components in the same places throughout the diagrams. Feel free to resolve this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some subgraphs, which I do think help. Good suggestion. They also broke the right to left setup, but I think left to right is fine now.

This shows editing during a transaction:

```mermaid
graph RL;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also wondering if this particular graph would benefit from being a sequence diagram instead. The arrows are positioned in a way that makes them a bit confusing and it's difficult to tell which actors in this process matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a sequence diagram and I have mixed feelings about it. I'll make a separate PR for that change so its easier to compare

Copy link
Contributor

Choose a reason for hiding this comment

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

I think with the subgraphs, it's probably not as necessary.


```mermaid
graph RL;
command["App's command callback"]--"completes transaction"-->transaction
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a "completes transaction" arrow here and in the following line which is a bit confusing for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the application's command completes the transaction (returns in the case of a synchronous transaction), that is information flow from the command callback to the transaction which kicks of this whole process causing the transaction to submit the changes. I'm not sure how to more clearly capture that. Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm maybe either replacing the first with "trigger transaction completion" or the second with something along the lines of "finalize edits into transaction"? I think it'll help as long as it's clear in what component the transaction is actually being completed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made some more changes. Maybe its more clear now.

@CraigMacomber CraigMacomber merged commit 99e4847 into microsoft:main Aug 22, 2022
@CraigMacomber CraigMacomber deleted the more-diagrams branch August 22, 2022 23:31
@github-actions
Copy link
Contributor

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds Issues related to distributed data structures base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants