Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion Orange/widgets/utils/itemmodels.py
Original file line number Diff line number Diff line change
Expand Up @@ -869,8 +869,25 @@ class DomainModel(VariableListModel):
ATTRIBUTES)
PRIMITIVE = (DiscreteVariable, ContinuousVariable)

def __init__(self, order=SEPARATED, placeholder=None,
def __init__(self, order=SEPARATED, separators=True, placeholder=None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, beware of changing the parameter order or inserting arguments within the existing arguments. If anybody constructed a DomainModel by calling DomainModel(DomainModel.Primitive, "(No color)") and not DomainModel(DomainModel.Primitive, placeholder="(No color)"), this change would break his/her code.

However, we seem to have always given these arguments with keywords.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also add a *, before them to make it explicit that they can only be used as keyword arguments.

@janezd janezd Oct 28, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Not just here.

  • It makes it easier to manipulate arguments without breaking backward compatibility.
  • It forces us to name arguments in calls, which makes the code more readable and prevents making some mistakes we've made in the past.

We should at least encourage adding a * in new functions.

valid_types=None, alphabetical=False, skip_hidden_vars=True, **kwargs):
"""

Parameters
----------
order: tuple or int
Order of attributes, metas, classes, separators and other options
separators: bool
If False, remove separators from `order`.
placeholder: str
The text that is shown when no variable is selected
valid_types: tuple
(Sub)types of `Variable` that are included in the model
alphabetical: bool
If true, variables are sorted alphabetically.
skip_hidden_vars: bool
If true, variables marked as "hidden" are skipped.
"""
super().__init__(placeholder=placeholder, **kwargs)
if isinstance(order, int):
order = (order,)
Expand All @@ -880,6 +897,8 @@ def __init__(self, order=SEPARATED, placeholder=None,
order = (None,) + \
(self.Separator, ) * (self.Separator in order) + \
order
if not separators:
order = [e for e in order if e is not self.Separator]
self.order = order
self.valid_types = valid_types
self.alphabetical = alphabetical
Expand Down
18 changes: 18 additions & 0 deletions Orange/widgets/utils/tests/test_itemmodels.py
Original file line number Diff line number Diff line change
Expand Up @@ -310,3 +310,21 @@ def test_filtering(self):
skip_hidden_vars=False)
model.set_domain(Domain(attrs))
self.assertEqual(list(model), disc)

def test_no_separators(self):
"""
GH-2697
"""
attrs = [ContinuousVariable(n) for n in "abg"]
classes = [ContinuousVariable(n) for n in "deh"]
metas = [ContinuousVariable(n) for n in "ijf"]

model = DomainModel(order=DomainModel.SEPARATED, separators=False)
model.set_domain(Domain(attrs, classes, metas))
self.assertEqual(list(model), classes + metas + attrs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about checking whether it works with separators=True?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually added this test before you responded.


model = DomainModel(order=DomainModel.SEPARATED, separators=True)
model.set_domain(Domain(attrs, classes, metas))
self.assertEqual(
list(model),
classes + [PyListModel.Separator] + metas + [PyListModel.Separator] + attrs)