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

[ENH] DomainModel: Don't Show Hidden Variables by Default #2690

Merged
merged 2 commits into from
Oct 18, 2017

Conversation

nikicc
Copy link
Contributor

@nikicc nikicc commented Oct 17, 2017

Issue

DomainModel shows all variables, even those, that are marked as hidden.

Description of changes

DomainModel shows only non-hidden variables by default. If skip_hidden_vars is set to False, all variables are shown.

Includes
  • Code changes
  • Tests
  • Documentation

@nikicc nikicc requested a review from janezd October 17, 2017 10:09
@@ -903,6 +904,8 @@ def set_domain(self, domain):
*(vars for i, vars in enumerate(
(domain.attributes, domain.class_vars, domain.metas))
if (1 << i) & section)))
if self.skip_hidden_vars:
to_add = filter_visible(to_add)
Copy link
Contributor

@janezd janezd Oct 17, 2017

Choose a reason for hiding this comment

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

to_add = list(filter_visible(to_add))

This is needed because of if to_add later on, which tests whether there is some more content and a separator needs to be added. Since filter_visible returns a generator, the condition is always True and a separator is added even when the generator would return nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be fixed now. Though, in the future, we might consider changing filter_visible to return a tuple instead of a generator.

Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

Fails existing tests. See the comment on how to fix it.

@janezd
Copy link
Contributor

janezd commented Oct 17, 2017

Could you add some tests?

Actually, I added them. (You're welcome. :) ).

I haven't added the fix for the bug from the comment. Squash it into your previous commit. :)

@nikicc nikicc force-pushed the domain-model-filter-visible branch from ce019e3 to 175887e Compare October 18, 2017 11:10
@codecov-io
Copy link

Codecov Report

Merging #2690 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2690      +/-   ##
==========================================
+ Coverage   75.81%   75.82%   +<.01%     
==========================================
  Files         338      338              
  Lines       59493    59512      +19     
==========================================
+ Hits        45106    45125      +19     
  Misses      14387    14387

@janezd janezd merged commit 8cbe98a into biolab:master Oct 18, 2017
@nikicc nikicc deleted the domain-model-filter-visible branch October 19, 2017 08:28
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.

3 participants