[FIX] EditDomain: Editing TimeVariable broke formatting #3101
Conversation
|
|
|
Looks ok to me! 👍 |
Orange/widgets/data/oweditdomain.py
Outdated
| var.name, | ||
| (), | ||
| attributes) | ||
| if var.is_time: |
There was a problem hiding this comment.
Any particular reason this is within an else statement?
There was a problem hiding this comment.
No, it would be really better to use elif.
Orange/widgets/data/oweditdomain.py
Outdated
| var.attributes.update(attrs) | ||
| if var.is_time: | ||
| var.have_date = kwargs['have_date'] |
There was a problem hiding this comment.
Have you considered adding have_date and have_time to TimeVariable constructor as keyword arguments?
There was a problem hiding this comment.
I was considering this but was afreide of breking something else, will add have_time and have_date as keyword arguments to TimeVariable constructor.
|
|
||
| # if continuous variable edit is used have_time and have_date is not | ||
| # copied and the time string format is changed | ||
| idx = self.widget.domain_view.model().index(4) |
There was a problem hiding this comment.
Can you try to use https://github.com/biolab/orange3/blob/master/Orange/widgets/tests/utils.py#L273 here?
| # copied and the time string format is changed | ||
| idx = self.widget.domain_view.model().index(4) | ||
| self.widget.domain_view.setCurrentIndex(idx) | ||
| editor = self.widget.editor_stack.findChild(ContinuousVariableEditor) |
There was a problem hiding this comment.
Not sure why this is tested her. Can ContinuousVariableEditor be used for editing TimeVariable by users?
| w.set_data(None) | ||
| self.assertEqual(w.name_edit.text(), "") | ||
| self.assertEqual(w.labels_model.get_dict(), {}) | ||
| self.assertIs(w.get_data(), None) |
There was a problem hiding this comment.
You might want to add some tests that assure that the new variable has the same value for have_date and have_time as source.
Orange/widgets/data/oweditdomain.py
Outdated
| if var.is_discrete: | ||
| editor_index = 0 | ||
| elif var.is_continuous: | ||
| elif var.is_time: |
There was a problem hiding this comment.
Can you add a comment why time variables need to be handled before numeric variables?
There was a problem hiding this comment.
TimeVariable is also continuous so we need to test if_time first. Will add comment.
Orange/widgets/data/oweditdomain.py
Outdated
| @@ -412,6 +449,7 @@ def __init__(self): | |||
| self.editor_stack = QStackedWidget() | |||
|
|
|||
| self.editor_stack.addWidget(DiscreteVariableEditor()) | |||
| self.editor_stack.addWidget(TimeVariableEditor()) | |||
There was a problem hiding this comment.
I would add this after ContinuousVariableEditor.
this way, the next diff should register as an addition of elif var.is_time as the elif var.is_continuous branch could remain unmodified
|
@kernc, are you ok with adding have_date and have_time kwargs to TimeVariable? They make construction of new variables where we already know what kind of datetime they contain much easier. |
astaric
changed the title
|
Was always in favor of setting parameters through the constructor. 👍 |
Issue
Fixes #2776
Description of changes
Add TimeVariableEditor and update variable_description and variable_from_description to correctly handle TimeVariables.
Includes