[FIX] Save data widget crash on no data #3311
Conversation
…anges" This reverts commit 246795b9bc2c343d4214cf42a986f2a6be40b732.
|
Thanks. Should there be a test added as well so this can't happen to other widgets? |
There was a problem hiding this comment.
The fix introduces a different bug. See the comments.
The bug occurred because the conditions in the code are too complex to be manageable. I had a feeling that some cases are not covered but needed some time to decipher which. I think you should simplify the conditions.
Better still: is this change really needed? The new code is much longer and more complex than the original. What do we gain by the change?
| if data is not None: | ||
| self.save_file() | ||
| if data is None: | ||
| return |
There was a problem hiding this comment.
self.data = data? Or, equivalently, self.data = None?
Orange/widgets/data/owsave.py
Outdated
| self.controls.filetype.insertItems(0, [item for item, _, supports_sparse in FILE_TYPES | ||
| if supports_sparse]) | ||
| else: | ||
| if (self.data is None and data.is_sparse()) or ( |
There was a problem hiding this comment.
These conditions do not cover the case self.data is None and not data.is_sparse().
Connect some sparse data to the widget. This will remove the formats that do not support sparse data. Then remove the signal to set self.data to None. Then connect some non-sparse data. After this, the widget will (incorrectly) show only formats for sparse data.
There was a problem hiding this comment.
I can't seem to replicate this bug.
I've tested like you described here, but it shows correct formats.
There was a problem hiding this comment.
Have you added self.data = data before line 132? This line is needed to ensure that self.data stores the current data and just not the last data seen by the widget. At the moment it may not matter, but it may introduce bugs in the future by defying expectations of what is in self.data.
I still don't understand the purpose of the change. However, even if there is/was no bug, the code is overly complicated and repetitive. I suppose the intention was to update the combo only when the sparsity of the new data is different from the previous. This translates to self.data is None or self.data.is_sparse() != data.is_sparse() (condition data is None is take care of earlier). This makes the code more readable and half shorter:
if self.data is None or self.data.is_sparse() != data.is_sparse():
self.controls.filetype.clear()
allowed = [item for item, _, supports_sparse in FILE_TYPES
if supports_sparse or not data.is_sparse()]
self.controls.filetype.insertItems(0, allowed)
self.filetype = allowed[0]
self.update_extension()
|
Could you squash the commits? |
Revert "Fix for crash on empty data, better handling on sparcisity changes" This reverts commit 246795b9bc2c343d4214cf42a986f2a6be40b732. Fix for empty data crash, better handling of sparse data Merge remote-tracking branch 'origin/owsave' into owsave
Codecov Report
@@ Coverage Diff @@
## master #3311 +/- ##
=========================================
- Coverage 82.57% 82.07% -0.5%
=========================================
Files 348 351 +3
Lines 61395 61922 +527
=========================================
+ Hits 50697 50824 +127
- Misses 10698 11098 +400 |
Orange/widgets/data/owsave.py
Outdated
| else: | ||
| self.controls.filetype.insertItems(0, [item for item, _, _ in FILE_TYPES]) | ||
| items = [item for item, _, _ in FILE_TYPES] | ||
| if not items == [self.controls.filetype.itemText(i) |
Orange/widgets/data/owsave.py
Outdated
| else: | ||
| self.controls.filetype.insertItems(0, [item for item, _, _ in FILE_TYPES]) | ||
| items = [item for item, _, _ in FILE_TYPES] |
There was a problem hiding this comment.
OK, that's one way to do it. It may be a matter of opinion, but isn't
items = [item for item, _, supports_sparse in FILE_TYPES
if supports_sparse or not data.is_sparse()]
more concise and shorter than if's with separate comprehensions? :)
Issue
Fixes #3256.
Description of changes
Does not crash on empty data anymore.
Includes