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.

Sign up for GitHub

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

Jump to bottom

Don't return TaffyResult when Taffy methods can't fail. #519

Open
gibbz00 opened this issue Jul 20, 2023 · 1 comment · May be fixed by #520
Open

Don't return TaffyResult when Taffy methods can't fail. #519

gibbz00 opened this issue Jul 20, 2023 · 1 comment · May be fixed by #520
Labels
breaking-change A change that breaks our public interface usability Make the library more comfortable to use

Comments

@gibbz00
Copy link

gibbz00 commented Jul 20, 2023

What problem does this solve or what need does it fill?

Reduces the amount of end user unwrap calls.

What solution would you like?

Simply remove the result wrapping for the following Taffy methods:

  • new_leaf
  • new_with_children
  • compute_layout
  • new_leaf_with_measure
  • remove
  • children
  • layout
  • style
  • dirty
@gibbz00 gibbz00 added the enhancement New feature or request label Jul 20, 2023
@nicoburns
Copy link
Collaborator

nicoburns commented Jul 20, 2023 •
edited
Loading

I think this generally makes sense. One thing I would point out that should probably be discussed before this is implemented is that I believe some of these methods are currently using array indexing to access node data (which will panic if the NodeId is invalid), and it may make sense for these to return errors instead.

Possibly we ought to have both panicking and result-returning variants of these methods?

gibbz00 added a commit to gibbz00/taffy that referenced this issue Jul 20, 2023
@gibbz00
Don't return TaffyResult when Taffy methods can't fail.
4a9ac5e 
Initial commit for: DioxusLabs#519.

Affected methods:

`new_leaf`, `compute_layout`, `new_leaf_with_measure`,
`new_with_children`, `remove`, `set_measure`, `add_child`,
`set_children`, `child_count`, `children`, `layout`, `set_style`,
`style`, `mark_dirty`, `dirty`.

Should be noted that many of the methods can still panic due to the
frequent use of direct array indexing.
@gibbz00 gibbz00 linked a pull request Jul 20, 2023 that will close this issue
Open
3 tasks
@alice-i-cecile alice-i-cecile added usability Make the library more comfortable to use breaking-change A change that breaks our public interface and removed enhancement New feature or request labels Jul 20, 2023
gibbz00 added a commit to gibbz00/taffy that referenced this issue Jul 20, 2023
@gibbz00
Don't return TaffyResult when Taffy methods can't fail.
45f1a9e 
Initial commit for: DioxusLabs#519.

Affected methods:

`new_leaf`, `compute_layout`, `new_leaf_with_measure`,
`new_with_children`, `remove`, `set_measure`, `add_child`,
`set_children`, `child_count`, `children`, `layout`, `set_style`,
`style`, `mark_dirty`, `dirty`.

Should be noted that many of the methods can still panic due to the
frequent use of direct array indexing.
@nicoburns nicoburns mentioned this issue Sep 15, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking-change A change that breaks our public interface usability Make the library more comfortable to use
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't return TaffyResult when Taffy methods can't fail. gibbz00/taffy
3 participants
@nicoburns @alice-i-cecile @gibbz00