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

[FIX] Ensure unique var names in file #4431

Merged
merged 4 commits into from
Feb 28, 2020

Conversation

AndrejaKovacic
Copy link
Contributor

@AndrejaKovacic AndrejaKovacic commented Feb 17, 2020

Issue

Fixes part of #4382
File widget allowed renaming vars with no regards for possible duplicates it may create.

Description of changes

As in other widgets, duplicates are checked by get_unique_name_duplicates and a warning is shown.

Includes
  • Code changes
  • Tests

@codecov
Copy link

codecov bot commented Feb 17, 2020

Codecov Report

Merging #4431 into master will increase coverage by 0.01%.
The diff coverage is 98.07%.

@@            Coverage Diff             @@
##           master    #4431      +/-   ##
==========================================
+ Coverage   83.13%   83.14%   +0.01%     
==========================================
  Files         268      268              
  Lines       53915    53928      +13     
==========================================
+ Hits        44820    44838      +18     
+ Misses       9095     9090       -5

@AndrejaKovacic AndrejaKovacic force-pushed the unique-names branch 2 times, most recently from 00c50a0 to 35231b7 Compare February 18, 2020 14:00
@janezd janezd self-assigned this Feb 19, 2020
@janezd
Copy link
Contributor

janezd commented Feb 20, 2020

This changes the user input -- and the change, at least on my Qt, isn't visible until I click Apply and then click one of variables.

I would change it so that the user either wouldn't be allowed to use the name that already exists (this would be a typical solution when renaming files) or I would keep the names set by the user as they are, and only rename the output variables, in the background (this is what most widgets do).

I am more inclined towards the latter. We will need this same thing in many other widgets, so perhaps it would make sense to write a function Orange.data.util.create_safe_domain(attributes, class_vars, variables) (name is up to discussion) that would get "proposed" names of attributes, class_vars and variables, rename them as needed and return a tuple (domain, renamed_variables).

@janezd
Copy link
Contributor

janezd commented Feb 20, 2020

@AndrejaKovacic, I'm so sorry that I did it again. :( After I realized my above comment makes no sense, I started poking around and changed the code here and there to see how this should be done ... and ... then it felt stupid to throw the changes away. I promise to discipline myself in the future.

You changed the code in the right place I think, but there were two problems. One is that it modifies the user's modifications, as I complained above. The other is that DomainEditor.get_domain used an attribute (renamed_variables) to return result. That is, renamed_variables is not a property of the object but just an information that is stored there to be retrieved after calling get_domain.

I changed your code so that it returns renamed variables as a part of result and so that it renames variables without changing the model.

@janezd janezd force-pushed the unique-names branch 6 times, most recently from ed06e3f to d9464f7 Compare February 24, 2020 18:47
@AndrejaKovacic
Copy link
Contributor Author

To break the current iteration, rename the attributes to:
iris (1) (1)
iris (1)
iris (1)
iris
iris

@janezd janezd force-pushed the unique-names branch 3 times, most recently from 8fe8acb to 12d1e09 Compare February 27, 2020 17:15
@janezd
Copy link
Contributor

janezd commented Feb 28, 2020

This time, the problem was that I didn't escape a regular expression, so it didn't properly parse names that included parentheses. It would be even more interesting if you named a variable "unclosed parentheses(".

@AndrejaKovacic AndrejaKovacic merged commit 1045e72 into biolab:master Feb 28, 2020
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.

2 participants