Add documentation, e.g. around bloom usage.#818
Conversation
luleyleo
left a comment
luleyleo
left a comment
There was a problem hiding this comment.
I did some nitpicking and questioning 😅
| Target::Widget(id) => { | ||
| recurse = child_ctx.base_state.children.contains(id); | ||
| // Recurse when the target widget could be our descendant. | ||
| // The bloom filter we're checking can return false positives. |
There was a problem hiding this comment.
The bloom filter we're checking can return false positives.
I think the renaming to may_contain should make it clear enough, as this comment has to be applied to too many places to stay consistent (and those were definitly to many tos 😅 ).
There was a problem hiding this comment.
Just the name may_contain will make it clear enough to someone who already knows the function or will go read its docs. Others who are just glancing at the code at the call site won't necessarily know.
I also don't think the three places this comment was added to is too many. Once we're up to 10 or something we can reconsider.
luleyleo
left a comment
luleyleo
left a comment
There was a problem hiding this comment.
Looks all good to me now, thanks!
|
On further thinking I moved the |
|
The force-pushing makes it a bit difficult to see what you've changed 😓 |
|
Yeah I removed the public doc changes for |
cmyr
left a comment
cmyr
left a comment
There was a problem hiding this comment.
Commented with some clarifications; feel free to incorporate this however you think is appropriate.
| /// This can return false positives, but never false negatives. | ||
| pub fn contains(&self, item: &T) -> bool { | ||
| /// Thus `true` means that the item may have been added - or not, | ||
| /// while `false` means that the item has definitely not been added. |
There was a problem hiding this comment.
I think these two lines are a strict restating of the preceding line; I think between the changed method name and the changed first line comment, this should be clear to our expected audience.
There was a problem hiding this comment.
I think it's worth stating it twice with different wording. The mistakes that result from not understanding this are extremely subtle and hard to catch.
xStrom
added
S-waiting-on-author
|
I removed the note I originally had added to I also rewrote the documentation for focus related methods/variants. Some of that stuff was strictly expired and no longer reflected reality. |
xStrom
added
S-needs-review
luleyleo
left a comment
luleyleo
left a comment
There was a problem hiding this comment.
Looks really nice!
I've been thinking about adding doc(hidden) to RouteFocusChanged and RouteWidgetAdded tho.
If I understand correctly, those are never really of interest to the user.
One disadvantage could be that they will still be completed by IDEs like Rust Analyzer, same applies however to the Debug... ones so this problem exists already.
Maybe hiding them and renaming to InternalRoutFocusChanged would be an option?
|
We could also do something like prefix them with |
|
I've created a new issue (#826) regarding the internal variants of |
3 participants
These are the simpler parts of #811 so that they can go on the merge train without waiting for more work there.