Skip to content

Unit test compute_constants()#184

Merged
alice-i-cecile merged 25 commits intoDioxusLabs:mainfrom
Weibye:test-aglo-constants
Jun 23, 2022
Merged

Unit test compute_constants()#184
alice-i-cecile merged 25 commits intoDioxusLabs:mainfrom
Weibye:test-aglo-constants

Conversation

@Weibye
Copy link
Copy Markdown
Collaborator

@Weibye Weibye commented Jun 19, 2022

Objective

One more step towards #92

This PR is a bit of a mixed bag, so let me know if I should split it up.

The objective of this PR is to set us up so that we can start testing the flexbox algorithm.

Context

While trying to cover the compute_constants() in tests, I discovered the code needed a bit refactoring and massaging in order to actually be testable.

Here I realized that the different resolve() methods can be abstracted to a "Resolve" trait, and following the convention of MaybeMath, we now have MaybeResolve and ResolveOrDefault.

These traits encapsulate behaviour where we need to resolve from a potentially context-dependent size / dimension into a context-independent size / dimensions. In other words:

  1. A size can be one (Dimension) or two dimensional (Size<Dimension> / Rect<Dimension>)
  2. These sizes can be fully defined on their own:
    1. Dimension::Points(n) tells us that this is n long
  3. Or sizes can be dependent on the size of its parent
    1. Dimension::Percent(n) tells us that this is n * parent.width long
    2. Hence "context-dependent"
  4. In order perform calculation of the node, we need the sizes to be fully independent of any parent context. This is where the resolve come in.

The only real difference between the two is what they output when they are unable to resolve the size

  • MaybeResolve will return a Option<f32> with a None as the fallback
  • ResolveOrDefault will return a f32 with the value 0.0 as the fallback

Changelog

  • Created a new module resolve which houses all code related code
  • Created the trait MaybeResolve
  • Create the trait ResolveOrDefault
  • Dimension.resolve() and Size<Dimension>.resolve() has been replaced by impl MaybeResolve
  • Changed the signature of compute_constants() to make it easier to test
  • Added #[must_use] on static constructors that was missing them
  • Added convenience constructors on Size<Dimension>, Rect<Dimension>
  • [BREAKING] Size<f32>.zero() has now become Size::<f32>::ZERO
  • [BREAKING] Point<f32>.zero() has now become Point::<f32>::ZERO
  • Added a new() constructor to Size<f32>
  • Removed Rect<T>.map as not used anymore

Feedback wanted

  • Does the traits make sense and make the code cleaner + easier to understand?
  • Docs help plz

@Weibye
Copy link
Copy Markdown
Collaborator Author

Weibye commented Jun 20, 2022

Updated the PR description to reflect the work done so far

@alice-i-cecile alice-i-cecile added code quality Make the code cleaner or prettier. breaking-change A change that breaks our public interface labels Jun 20, 2022
@Weibye Weibye marked this pull request as ready for review June 23, 2022 17:49
@Weibye Weibye requested a review from TimJentzsch June 23, 2022 18:33
Copy link
Copy Markdown
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Are you happy with this?

@Weibye
Copy link
Copy Markdown
Collaborator Author

Weibye commented Jun 23, 2022

Looks good to me now. Are you happy with this?

Yes! I'm very happy with how this turned out, and I think this is the right place to stop ^^

@alice-i-cecile alice-i-cecile enabled auto-merge (squash) June 23, 2022 18:41
@alice-i-cecile alice-i-cecile merged commit b298fd6 into DioxusLabs:main Jun 23, 2022
@Weibye Weibye deleted the test-aglo-constants branch June 23, 2022 18:43
mcrvaz pushed a commit to mcrvaz/taffy that referenced this pull request Jun 24, 2022
* Begin testing algo-constants

* Separating out resolve into own trait + module

* Fix tests

* Add convenience methods to Size

* Move tests to resolve

* Add tests for maybe_resolve Size<Dimension>

* Move resolve tests to resolve.rs

* Rewording parent -> context

* Add last test

* Add tests for resolve_or_default for Dimension

* Adding rect constructors

* Using consts instead of constructors

* from_some() -> new()

* Add note to releases

* Turning constructors to consts

* Rect convenience constructors

* Add test-suite for rect resolve_or_default

* Apply suggestions from code review

Co-authored-by: Alice Cecile <[email protected]>

* Fix clippy lints & add docs

* Add tests for Rect -> Option -> Rect

* Finish testing algo-consts

* Apply suggestions from alice

* Tweak RELEASES.md wording

Co-authored-by: Alice Cecile <[email protected]>
jkelleyrtp pushed a commit that referenced this pull request Oct 10, 2022
* Begin testing algo-constants

* Separating out resolve into own trait + module

* Fix tests

* Add convenience methods to Size

* Move tests to resolve

* Add tests for maybe_resolve Size<Dimension>

* Move resolve tests to resolve.rs

* Rewording parent -> context

* Add last test

* Add tests for resolve_or_default for Dimension

* Adding rect constructors

* Using consts instead of constructors

* from_some() -> new()

* Add note to releases

* Turning constructors to consts

* Rect convenience constructors

* Add test-suite for rect resolve_or_default

* Apply suggestions from code review

Co-authored-by: Alice Cecile <[email protected]>

* Fix clippy lints & add docs

* Add tests for Rect -> Option -> Rect

* Finish testing algo-consts

* Apply suggestions from alice

* Tweak RELEASES.md wording

Co-authored-by: Alice Cecile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change A change that breaks our public interface code quality Make the code cleaner or prettier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants