Document entire crate by alice-i-cecile · Pull Request #96 · DioxusLabs/taffy

alice-i-cecile · 2022-06-08T20:19:58Z

Objective

docs are great
this crate is confusing to use
this crate is challenging to contribute to
by documenting the crate we'll unearth yet-more tech debt

Fixes #22.

Status

deny missing docs
document internals
document public API
document flexbox algorithm parameters in style.rs

alice-i-cecile · 2022-06-08T20:20:42Z

Help very much wanted here; if you want to add docs before I'm done, please make a PR to this branch!

If you spot something confusing or wrong, leave a comment :)

alice-i-cecile · 2022-06-09T03:04:47Z

Definitely struggling a bit to get the details right for the style.rs file. We'll get there, slowly, but that's the place that needs the most careful review.

TimJentzsch

Did a first pass on this, mainly for typos.
I also wrote down some possibilities for future improvements in other PRs.

This PR will definitely be an important milestone to make this crate more usable, great effort!

src/node.rs

src/lib.rs

src/geometry.rs

src/node.rs

src/geometry.rs

src/number.rs

src/forest.rs

src/style.rs

src/forest.rs

src/geometry.rs

src/node.rs

src/style.rs

Weibye · 2022-06-09T13:27:21Z

So first readthrough was with perhaps too little understanding of flexbox. Then looking at it again for a second go:

When it comes to documentation and naming structs / methods / parameters that relates to flexbox specifically, we should make sure to keep as close as possible with the specs.

This allows users that already know flexbox to know exactly what they are looking at
Allows new users to learn the ins and outs of flexbox instead of just learning how flexbox-in-sprawl-works
Focus our documentation efforts to teach the concepts of flexbox with links to the spec every now and then should some concepts require more details or to encourage further reading.

This would apply for flexbox.rs and style.rs

This might be obvious but I'd thought I'd mention it regardless.

src/style.rs

Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com>

alice-i-cecile · 2022-06-09T15:00:31Z

You all rock; thanks for all the feedback. This collaboration is exactly why I wanted to get a draft PR up ASAP.

Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de>

Weibye · 2022-06-09T19:13:05Z

My PR into this is now ready for review so please head on over there and speak your mind :) alice-i-cecile#2

It attempts to finish up documentation of style.rs

alice-i-cecile · 2022-06-09T19:22:38Z

Oh I see! @Weibye we duplicated work; can you add any relevant changes as comments on this PR? Or perhaps rebase your PR?

Weibye · 2022-06-09T19:30:29Z

Oh I see! @Weibye we duplicated work; can you add any relevant changes as comments on this PR? Or perhaps rebase your PR?

On it :)

Weibye

I made a complete mess of my PR, but here you have the useful pieces as comments :)

src/style.rs

src/node.rs

Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com>

Weibye

📖 I approve this

TimJentzsch

I really like the documentation for style.rs so far, detailed and with links to the specs. Great job!

TimJentzsch · 2022-06-09T20:19:51Z

src/forest.rs

 use crate::style::Style;
 use crate::sys::{new_vec_with_capacity, ChildrenVec, ParentsVec, Vec};

+/// Layout information for a given [`Node`](crate::node::Node)


This is the smallest nit, but I noticed that you do not include a full stop at the end of the first line of the doc comments, but I did in flexbox.rs. Are there style guidelines/conventions for this in Rust? Do we care enough to make this consistent? (If not it's totally fine to ignore)

I don't care enough right now; if you'd like feel free to put together a style guide PR.

TimJentzsch · 2022-06-09T20:21:21Z

src/forest.rs

    }

+    /// Create the data for a new node
+    // TODO: why is this different from new_leaf?


This function doesn't allow to provide a MeasureFunc, while new_leaf does.
If we want to change something here I think we should also create an issue such that the TODO isn't buried in the code.

Yeah. I think that MeasureFunc is just a bad design, TBH.

TimJentzsch · 2022-06-09T20:23:07Z

src/forest.rs

 impl Forest {
-    pub fn with_capacity(capacity: usize) -> Self {
+    /// Creates a new [`Forest`] that can store up to `capacity` [`Nodes`](crate::node::Node) before needing to be expanded
+    pub(crate) fn with_capacity(capacity: usize) -> Self {


I noticed that you changed this from pub to pub(crate), is this a breaking change? To be honest, this crate is the first time I have ever seen pub(crate) so I'm not entirely sure what it does.

No, it's not. The type is not public, so the method cannot be used outside the crate anyways.

pub(crate) says "anything within this crate can use this type, but nothing else". pub(super) is the other variant you might run across, which restricts visibility to the parent module.

(pub(crate) would be somewhat similar to internal in C# if you are familiar with that, @TimJentzsch)

src/geometry.rs

src/node.rs

Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de>

…sing-docs

alice-i-cecile · 2022-06-10T16:55:55Z

I'm going to merge this now; feel free to make any further corrections in new PRs :)

* Deny missing docs :3 * Module level docs * Document geometry.rs * Document layout.rs * Document prelude.rs * Document lib.rs * Document node.rs * Document number.rs * Partial docs for style.rs * Warn that padding broken :( * Add docs to forest.rs * Fix typo Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com> * Fix docs for geometry.rs * Typo fix Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de> * Typo fix Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de> * Revert non-exhaustive annotation Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de> * Rename unclear method names on Rect * Clarify wording of mark_dirty field Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de> * Clarify wording of mark_dirty field Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de> * Use `parent` as the argument name over `node` in methods on `Forest` * index -> child_index * Fix incorrect comment * More docs for style.rs * Fix merge error * Document JustifyContent * Finish documenting style.rs * Small improvements to style.rs docs Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com> * Typo fix Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com> * Document sys.rs * Document more of forest.rs * Docs tweaks Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de> * Typos Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de> * Caching notes * Complete internal documentation * Cargo fmt Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com> Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de>

alice-i-cecile added 3 commits June 8, 2022 15:37

Deny missing docs :3

d6c3a1e

Module level docs

559ac6a

Merge branch 'main' into deny-missing-docs

3065802

alice-i-cecile added the documentation Improvements or additions to documentation label Jun 8, 2022

alice-i-cecile added 6 commits June 8, 2022 17:54

Document geometry.rs

6dfc13b

Document layout.rs

851a7fd

Document prelude.rs

37245f3

Document lib.rs

b4c118d

Document node.rs

3541aa5

Document number.rs

cce66c0

alice-i-cecile requested review from TimJentzsch and Weibye June 8, 2022 23:32

Partial docs for style.rs

82e478a

alice-i-cecile mentioned this pull request Jun 9, 2022

Return a Result from Taffy::remove #106

Merged

alice-i-cecile added 3 commits June 8, 2022 21:07

Warn that padding broken :(

ff8f13f

Add docs to forest.rs

058faa2

Merge branch 'main' into deny-missing-docs

0afc9bd

TimJentzsch reviewed Jun 9, 2022

View reviewed changes

Weibye reviewed Jun 9, 2022

View reviewed changes

src/node.rs Outdated Show resolved Hide resolved

src/style.rs Outdated Show resolved Hide resolved

Weibye reviewed Jun 9, 2022

View reviewed changes

src/style.rs Outdated Show resolved Hide resolved

Weibye mentioned this pull request Jun 9, 2022

Adding docs to style.rs alice-i-cecile/taffy#2

Closed

Fix typo

75546cc

Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com>

alice-i-cecile and others added 3 commits June 9, 2022 12:24

Fix docs for geometry.rs

21c5fb5

Typo fix

ac1a88c

Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de>

Typo fix

f333976

Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de>

Finish documenting style.rs

ba44c89

alice-i-cecile requested review from TimJentzsch, Weibye, colepoirier and sixfold-origami June 9, 2022 19:20

Weibye reviewed Jun 9, 2022

View reviewed changes

Weibye mentioned this pull request Jun 9, 2022

style::Display is not according to specifications #115

Closed

Weibye reviewed Jun 9, 2022

View reviewed changes

src/node.rs Outdated Show resolved Hide resolved

alice-i-cecile and others added 3 commits June 9, 2022 16:25

Small improvements to style.rs docs

b870891

Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com>

Typo fix

3a33105

Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com>

Merge branch 'main' into deny-missing-docs

1a716b6

alice-i-cecile marked this pull request as ready for review June 9, 2022 20:40

alice-i-cecile added this to the Release new crate milestone Jun 9, 2022

Weibye approved these changes Jun 9, 2022

View reviewed changes

alice-i-cecile added 2 commits June 9, 2022 16:50

Document sys.rs

6806b43

Document more of forest.rs

7e53765

TimJentzsch reviewed Jun 9, 2022

View reviewed changes

alice-i-cecile and others added 7 commits June 9, 2022 21:00

Docs tweaks

8b2335d

Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de>

Typos

ec7a460

Co-authored-by: TimJentzsch <tim-jentzsch@gmx.de>

Caching notes

410c807

Merge remote-tracking branch 'origin/deny-missing-docs' into deny-mis…

cc72d0a

…sing-docs

Merge branch 'main' into deny-missing-docs

d50fdd4

Complete internal documentation

dba84c8

Cargo fmt

cb9ee01

alice-i-cecile merged commit de9123e into DioxusLabs:main Jun 10, 2022

Conversation

alice-i-cecile commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Status

Uh oh!

alice-i-cecile commented Jun 8, 2022

Uh oh!

alice-i-cecile commented Jun 9, 2022

Uh oh!

TimJentzsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Weibye commented Jun 9, 2022

Uh oh!

Uh oh!

alice-i-cecile commented Jun 9, 2022

Uh oh!

Weibye commented Jun 9, 2022

Uh oh!

alice-i-cecile commented Jun 9, 2022

Uh oh!

Weibye commented Jun 9, 2022

Uh oh!

Weibye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Weibye left a comment

Choose a reason for hiding this comment

Uh oh!

TimJentzsch left a comment

Choose a reason for hiding this comment

Uh oh!

TimJentzsch Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

TimJentzsch Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

TimJentzsch Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jun 10, 2022

alice-i-cecile commented Jun 8, 2022 •

edited

Loading