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

Domain: Prevent constructing domain with non-unique variable names #4382

Closed
VesnaT opened this issue Jan 30, 2020 · 6 comments · Fixed by #4578 or #4760
Closed

Domain: Prevent constructing domain with non-unique variable names #4382

VesnaT opened this issue Jan 30, 2020 · 6 comments · Fixed by #4578 or #4760
Assignees

Comments

@VesnaT
Copy link
Contributor

VesnaT commented Jan 30, 2020

Is your feature request related to a problem? Please describe.
We have an agreement to construct domains with unique variable names, but there are no checks for that in the code.

Describe the solution you'd like
Raise an error when constructing a domain with duplicated variable names.

Describe alternatives you've considered
Create unique variable names under the hood when constructing the domain.

@janezd
Copy link
Contributor

janezd commented Jan 30, 2020

I support raising an exception because we should prefer failing early. The cost of this will be unhandled exceptions at various places, but this is better than unreported errors in various places (and miscomputations later).

I don't like the under-the-hood idea.

@janezd janezd added the snack This will take an hour or two label Jan 31, 2020
@VesnaT
Copy link
Contributor Author

VesnaT commented Jan 31, 2020

Before we accept this enhancement, it is necessary to fix all widgets that may construct a new domain (which seems impossible).

@AndrejaKovacic
Copy link
Contributor

AndrejaKovacic commented Feb 14, 2020

Problematic widgets

Data widgets

Visualize

Unsupervized

Evaluate

@VesnaT
Copy link
Contributor Author

VesnaT commented Feb 21, 2020

Another proposal: Prevent constructing domains from tuples and lists. Create custom collection e.g. SortedSet. The collection is returned by get_unique_names. The purpose is to force the user to always use the get_unique_names when constructing the domain.

@VesnaT VesnaT added the needs discussion Core developers need to discuss the issue label Feb 21, 2020
@janezd
Copy link
Contributor

janezd commented Feb 21, 2020

Clever idea. But it would be backwards incompatible. Plus ... think about tests with hardcoded names - which are obviously unique. I'd hate having to call get_unique_names there. Plus if everybody just called get_unique_names, this is not much different from Domain calling it itself, I guess.

@janezd janezd removed snack This will take an hour or two needs discussion Core developers need to discuss the issue suggestion labels Feb 27, 2020
@NejcDebevec NejcDebevec self-assigned this Mar 6, 2020
@janezd
Copy link
Contributor

janezd commented Apr 19, 2020

I usually remember to edit "Partially fixes ..." to "Fixes (partially) ..." to prevent closing of an issue. In #4578 I forgot. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants