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

Move link to make document a bit easier to follow. #15895

Closed
wants to merge 1 commit into from

Conversation

adam-azarchs
Copy link
Contributor

Move the link to #accessing-attributes-with-transitions
from the section about how to define 1:2 transitions into the section
on how to attach an outgoing transition to an attribute, since that's
the section people are most likely to be reading when they need to
see that link.

This got me, leading to me opening #15892, because I wasn't implementing a 1:2+ transition and hadn't kept reading to the end of the document.

Move the link to `#accessing-attributes-with-transitions`
from the section about how to define 1:2 transitions into the section
on how to attach an outgoing transition to an attribute, since that's
the section people are most likely to be reading when they need to
see that link.
@sgowroji sgowroji added type: documentation (cleanup) team-Configurability Issues for Configurability team awaiting-review PR is awaiting review from an assigned reviewer labels Jul 18, 2022
@gregestren
Copy link
Contributor

I appreciate your confusion but I think there's an important conceptual connection between the "Defining 1:2+ transitions" and "Accessing attributes with transitions" sections. They're both explaining how to write the transition's implementation function.

"Outgoing edge transitions" is, of course, focused on attaching the transition itself.

I 100% support anything that can make the docs cleaner and less overwhelming. But I'd like to dig some more on these subtleties. What's the full logical path you follow? i.e. to make any of this work you have to know a) how to define a transition implementation b) how to reach the attributes in the implementation and c) how to attach the transition. If you exclusively read the "Outgoing edge transitions" section it doesn't cover enough of that.

I hope what I'm saying makes sense. I appreciate the opportunity to trace through these docs from someone else's vantage point.

@gregestren gregestren self-assigned this Jul 19, 2022
@gregestren gregestren removed the awaiting-review PR is awaiting review from an assigned reviewer label Jul 19, 2022
@adam-azarchs
Copy link
Contributor Author

I appreciate your confusion but I think there's an important conceptual connection between the "Defining 1:2+ transitions" and "Accessing attributes with transitions" sections. They're both explaining how to write the transition's implementation function.

While the code examples do include transition functions, the text of that section is exclusively discussing the implementation of a rule which has attached an outgoing transition. That's why it opens with a link back to the section on attaching a transition.

The direction I'm coming at this from is writing a 1:1 transition, which meant I never read the section on 1:2+ transitions, and thus missed that there was a separate section discussing how to access the attributes, and thus was confused why the attribute suddenly turned into a list of targets instead of just a single target. The idea with this change is that anyone who is going to want to access an attribute with a transition will always have to first attach the transition to the attribute, but they might not have any interest on reading up on 1:2+ transitions.

I 100% support anything that can make the docs cleaner and less overwhelming. But I'd like to dig some more on these subtleties. What's the full logical path you follow? i.e. to make any of this work you have to know a) how to define a transition implementation b) how to reach the attributes in the implementation and c) how to attach the transition. If you exclusively read the "Outgoing edge transitions" section it doesn't cover enough of that.

The section on 1:2+ transitions doesn't provide any useful context for a someone who is writing a 1:1 transition. If that's what you're doing (like I was) then you'd read the Defining section (which explains 1:1 transitions), then skip the discussion on 1:2+ before reading about how to attach the transition. Then there's several sections which aren't of interest, signaling to me that I'd read far enough to do what I came there to figure out how to do. That attaching a transition would change the type of value of the attribute is actually pretty surprising behavior, at least if you're focused on the 1:1 case - it's not like passing other values for cfg will do that.

@gregestren
Copy link
Contributor

I appreciate your confusion but I think there's an important conceptual connection between the "Defining 1:2+ transitions" and "Accessing attributes with transitions" sections. They're both explaining how to write the transition's implementation function.

While the code examples do include transition functions, the text of that section is exclusively discussing the implementation of a rule which has attached an outgoing transition. That's why it opens with a link back to the section on attaching a transition.

You're right. I read the logic wrong. Both transition and rule implementations can read attributes. I was referring to the wrong one.

Copy link
Contributor

@gregestren gregestren left a comment

Choose a reason for hiding this comment

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

Thanks for the elaboration. All makes sense to me.

@gregestren gregestren added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jul 22, 2022
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jul 26, 2022
@adam-azarchs adam-azarchs deleted the patch-2 branch September 20, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants