Improve lifecycle docs #76021
Improve lifecycle docs #76021
Conversation
flutter-dashboard
bot
added
the
framework
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| /// same [owner]. | ||
| /// | ||
| /// If you override this, make sure your method starts with a call to | ||
| /// `super.attach(owner)`. |
There was a problem hiding this comment.
The Flutter API docs voice avoids telling the reader what to do -- because we don't know if the reader wrote the code they care about, or if they are just trying to learn how someone else's code works. So we use the passive voice or talk about the code itself, without reference to the reader or the programmer, as in "Implementations of this method should start with a call to the superclass, as in `super.attach(owner)`.".
There was a problem hiding this comment.
(comments along these lines apply below as well)
There was a problem hiding this comment.
(we should fix the existing text too)
There was a problem hiding this comment.
@Hixie Thanks, makes sense! Adjusted it in the new and existing docs.
|
(test exempt: documentation changes) |
@Hixie
Many occurences are indeed violations of the rule, which would make writing a test feasible. However, there are also a lot of occurences like the following:
In this case, I do not think that "you" really violates the voice policy because it is more of a passive "you" in this case. Furthermore, I do think that using "you" in general should be forbidden because it would likely limit the language in the docs beyond the voice. |
|
Wow thanks for looking into that. I agree that that's a little beyond the scope of this PR! :-) |
Fixes flutter#115525. On [AbstractNode.detach] and its two progeny [RenderNode.detach] and [Layer.detach], the docs said both to call the inherited method before detaching children, and to end by doing so. The former advice is what's enforced by an assertion in the base implementation, so cut out the other. The corresponding [attach] methods redundantly said twice to call the inherited method first, so cut the redundancy. Leave in place the version more recently added (in flutter#76021), because that PR shows the old version must have been easy to overlook.
…#130688) Fixes #115525. On [AbstractNode.detach] and its two progeny [RenderNode.detach] and [Layer.detach], the docs said both to call the inherited method before detaching children, and to end by doing so. The former advice is what's enforced by an assertion in the base implementation, so cut out the other. The corresponding [attach] methods redundantly said twice to call the inherited method first, so cut the redundancy. Leave in place the version more recently added (in #76021), because that PR shows the old version must have been easy to overlook.
…flutter#130688) Fixes flutter#115525. On [AbstractNode.detach] and its two progeny [RenderNode.detach] and [Layer.detach], the docs said both to call the inherited method before detaching children, and to end by doing so. The former advice is what's enforced by an assertion in the base implementation, so cut out the other. The corresponding [attach] methods redundantly said twice to call the inherited method first, so cut the redundancy. Leave in place the version more recently added (in flutter#76021), because that PR shows the old version must have been easy to overlook.
4 participants
This PR aims at improving the docs for lifecycle methods in
AbstractNode,Element, andState.For these methods, it is usually crucial that the paradigm of calling the inherited method first or last is followed. The reason for that is that there might be some preliminary setup or disposal of resources that has to happen first/last.
Not following it is an easy source of error when adding mixins or not looking up all ancestor overrides. I assume this is also why at this time the notes are already present in
State- I simply added them toAbstractNodeandElementas well.I noticed that some people writing
RenderObjectsubclasses are not following it, so establishing a paradigm via the docs should be beneficial.There is no issue for this because this is just my attempt at improving the docs - maybe my assumptions are wrong after all.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.