ttkwidgets
ttkwidgets copied to clipboard
TkinterEP
(un)check_all
Codecov Report
Merging #96 (21857ed) into master (1a8ce0c) will increase coverage by
0.01%. The diff coverage is100.00%.
@@ Coverage Diff @@
## master #96 +/- ##
==========================================
+ Coverage 89.51% 89.53% +0.01%
==========================================
Files 43 43
Lines 4026 4032 +6
==========================================
+ Hits 3604 3610 +6
Misses 422 422
| Impacted Files | Coverage Δ | |
|---|---|---|
| ttkwidgets/checkboxtreeview.py | 93.18% <100.00%> (+0.32%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 1a8ce0c...21857ed. Read the comment docs.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Nice! Looks good to me!
Just a few suggestions to make it less boilerplate. (Something's broken here, so I can't make a real review suggestion)
I think the methods could be simplified. Currently the root item is handled separately, but it could be moved to aux with some simple modification.
def check_all(self):
"""Check all items."""
def aux(item):
if item:
self.change_state(item, "checked")
children = self.get_children(item)
for c in children:
aux(c)
aux("")
Maybe also delete the duplicate methods, and use functools.partialmethod instead.
def _set_state_to_all(self, state):
def aux(item):
if item:
self.change_state(item, state)
children = self.get_children(item)
for c in children:
aux(c)
aux("")
check_all = functools.partialmethod(_set_state_to_all, "checked")
uncheck_all = functools.partialmethod(_set_state_to_all, "unchecked")
My only comment is that I really want this funciton soon :-) I was going to have to write soemthing as I'm using one of thse and need the check and uncheck all available.
Nice! Looks good to me!
Just a few suggestions to make it less boilerplate. (Something's broken here, so I can't make a real review suggestion)
I think the methods could be simplified. Currently the root item is handled separately, but it could be moved to
auxwith some simple modification.def check_all(self): """Check all items.""" def aux(item): if item: self.change_state(item, "checked") children = self.get_children(item) for c in children: aux(c) aux("")Maybe also delete the duplicate methods, and use
functools.partialmethodinstead.def _set_state_to_all(self, state): def aux(item): if item: self.change_state(item, state) children = self.get_children(item) for c in children: aux(c) aux("") check_all = functools.partialmethod(_set_state_to_all, "checked") uncheck_all = functools.partialmethod(_set_state_to_all, "unchecked")
Oh, is something broken? I used this in my own project and didn't run into any issues
No, it isn't broken. These are just suggestions to improve the code quality, and make it less repetitive.
Anything on my end holding this up?
Not really. Tests are missing for the two new methods, but those are not necessarily your job. It's simply that the project maintainers don't have the time to work on it (I'd willingly do it, but I don't have write access currently)
@RedFantom I have made the change you requested. Anything on my end holding this up?
Why would you do that?
Since I have to wait for @RedFantom to see that I made the changes he/she requested
Yes, you have made the changes he requested, although for some reason Github doesn't recognize it and won't let me merge it.
Yes, you have made the changes he requested, although for some reason Github doesn't recognize it and won't let me merge it.
So if I withdraw it and make a new one then you could merge it, right?
Well, that could work, but that still wouldn't mean, that it will be released to Pypi, so I think it's better to wait for @RedFantom.
