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.

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

Add a clear method that removes all children. #1893

Merged
merged 4 commits into from Apr 24, 2023
Merged

Conversation

malikoth
Copy link
Contributor

@malikoth malikoth commented Apr 24, 2023

Fixes #1828

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Signed-off-by: Kyle Rich <yutakafrog@gmail.com>
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This implementation looks good; now we just need the rest of the owl :-)

Two pieces are missing:

  1. A change note - a file in the changes directory that provides the release note for this improvement. See the contribution guide for details on what is needed
  2. A test. core/tests/widgets/test_base.py has a suite of tests validating that child removal works; we need an additional set of tests for this new API. We need to validate that clear works for (a) a widget that has children, (b) a widget that can have children, but doesn't; and (c) a leaf node that can't have children.

Signed-off-by: Kyle Rich <yutakafrog@gmail.com>
Signed-off-by: Kyle Rich <yutakafrog@gmail.com>
@malikoth
Copy link
Contributor Author

Yup, just working through the contribution guide, little bit at a time. :)

@malikoth
Copy link
Contributor Author

The base widget documentation includes this block:

Reference
---------

.. autoclass:: toga.widgets.base.Widget
   :members:
   :undoc-members:
   :inherited-members:

Am I to understand that this will automatically slurp up the docstring of my new method and I don't need to manually do anything for this new method to be documented?

Signed-off-by: Kyle Rich <yutakafrog@gmail.com>
@malikoth
Copy link
Contributor Author

@freakboy3742 I think this is ready for re-review. よろしくお願いします!

@malikoth
Copy link
Contributor Author

malikoth commented Apr 24, 2023

OK, so the Travertino Widget.clear docs say that calling clear on a leaf node raises ValueError. My code does not do that, but then again... despite what the docs say, neither does Widget.clear. What is the best resolution?

Raising the error is preferable to silently doing a noop, right?

Copy link
Member

@freakboy3742 freakboy3742 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! Thanks for the fix!

@freakboy3742 freakboy3742 merged commit bc44914 into beeware:main Apr 24, 2023
43 checks passed
@wafaa09
Copy link

wafaa09 commented Jun 6, 2023

Hello! I don't mean to be inpatient, but if this feature will be done soon, I also don't want to spend time completing my project when this is exactly what I need! Is there a timeline for completion? I saw that there were a couple things left to do, but not sure how long that will take.
Thank you!

@freakboy3742
Copy link
Member

@wafaa09 Umm... the PR has been merged. It's done. It hasn't been released in a stable release of Toga, but there's no more work to be done. If you want a pre-release version you can install the main branch of Toga from the git repository.

@wafaa09
Copy link

wafaa09 commented Jun 6, 2023

Awesome, thank you for letting me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a clear() method to remove all children
3 participants