[FIX] Edit Domain: Prevent duplicate variable names#2146
[FIX] Edit Domain: Prevent duplicate variable names#2146lanzagar merged 2 commits intobiolab:masterfrom jerneju:gh-2143-editdomain-duplicate
Conversation
jerneju
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2146 +/- ##
==========================================
+ Coverage 72.13% 72.14% +0.01%
==========================================
Files 318 318
Lines 54691 54717 +26
==========================================
+ Hits 39450 39476 +26
Misses 15241 15241Continue to review full report at Codecov.
|
janezd
left a comment
janezd
left a comment
There was a problem hiding this comment.
-
The widget should indicate that the name is duplicated instead of silently ignoring the change.
-
The user can still assign the same new name to two attributes. I can rename all attributes to "foo".
Orange/widgets/data/oweditdomain.py
Outdated
| self.main_form.addRow("Labels:", vlayout) | ||
|
|
||
| def set_data(self, var): | ||
| def set_data(self, var, var_names=None): |
There was a problem hiding this comment.
Using var_names=() as default would simplify is_same, wouldn't it?
Orange/widgets/data/oweditdomain.py
Outdated
| self.main_form.addRow("Values:", self.values_edit) | ||
|
|
||
| def set_data(self, var): | ||
| def set_data(self, var, var_names=None): |
There was a problem hiding this comment.
Also here, you can probably replace None with ().
Orange/widgets/data/oweditdomain.py
Outdated
| except Exception: | ||
| pass | ||
| current.set_data(None) | ||
| current.set_data(None, None) |
There was a problem hiding this comment.
And then () (or nothing) instead of the second None, too.
Orange/widgets/data/oweditdomain.py
Outdated
| temp_var_names.remove(self.var.name) | ||
| else: | ||
| temp_var_names = [] | ||
| return ((name in temp_var_names) |
There was a problem hiding this comment.
This function checks whether it is necessary to change the domain. You shouldn't abuse it to check whether it's legal to change the domain. (See the general comment.)
jerneju
changed the title
Orange/widgets/data/oweditdomain.py
Outdated
|
|
||
| box.layout().addWidget(self.editor_stack) | ||
|
|
||
| self.Warning.add_message( |
There was a problem hiding this comment.
I think two variables with the same name should produce an error and require a fix before proceeding (instead of displaying a warning and working on)
There was a problem hiding this comment.
Well, @janezd has different opinion (see above).
There was a problem hiding this comment.
I think @janezd didn't like that the change was ignored without a message. I don't see anything about whether the message should be a warning or an error.
We do not want to allow duplicate names (which is not just a cosmetic guideline, it can cause serious problems in the code, because we can index by names etc), so the widget should not output incorrect data. In that case an error is the only appropriate type of message.
Orange/widgets/data/oweditdomain.py
Outdated
| self.domain_model[self.selected_index] = new_var | ||
|
|
||
|
|
||
| var_names = [vn.name for vn in self.domain_model] |
There was a problem hiding this comment.
This is now checked only on variable changes. If I see the warning and reset a variable to the original, the warning does not disappear (the reset functions don't reach this code).
Maybe the check could be moved to commit:
- if everything is ok, remove the error and proceed
- if not, show the error and send None
|
Also, editing a variable name and pressing enter does not work... Maybe that's a separate issue, but if it is easy to fix it can perhaps be added here? |
|
@lanzagar: Yes, pressing enter is another issue. |
White spaces in the beginning and in the end of the name are removed. Empty names are not allowed. GH-2143
lanzagar
changed the title
4 participants
Issue
Edit Domain enables us to write duplicate names and that should be allowed. However user should be informed about that.
#2143
Description of changes
User is warned when writing a duplicate name. Also, empty spaces are removed in the beginning and in the end of a new name.
Includes