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

OWDataSets: refactor code, move some functionalities to new methods #2950

Merged
merged 1 commit into from
Mar 16, 2018

Conversation

JakaKokosar
Copy link
Member

Issue

biolab/orange3-single-cell#121

Description of changes

Split large chunk of code into smaller methods, so we can easily override them.
Moved item model creation in a separate method.

Includes
  • Code changes
  • Tests
  • Documentation

@JakaKokosar
Copy link
Member Author

@lanzagar

self._header_labels = [header['label'] for _, header in self.HEADER_SCHEMA]
self._html_description_tags = ['year', 'source', 'title', 'description', 'seealso', 'references']

self._header_index = namedtuple('_header_index', [*[info_tag for info_tag, _ in self.HEADER_SCHEMA]])
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a syntax error on older python versions (as reported by failing tests).
But also, I don't see a reason for unpacking and repacking the list...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, forgot to remove this when I was testing something.


# add missing variables to the namespace
# not needed if all info files have the same JSON structure
for tag in self._html_description_tags:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not all variables are in _html_description_tags, e.g. one test fails, because 'size' is missing (which is in _header_index, but not in _html_description_tags.
Also, did you make sure that it is OK to set all missing things to None? Previously, some default values were different, e.g. [] for 'references'.

@JakaKokosar JakaKokosar force-pushed the master branch 3 times, most recently from 8ad5494 to 4fd7627 Compare March 13, 2018 11:15
@codecov-io
Copy link

codecov-io commented Mar 13, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2950      +/-   ##
==========================================
+ Coverage   81.86%   81.86%   +<.01%     
==========================================
  Files         329      329              
  Lines       56770    56782      +12     
==========================================
+ Hits        46474    46486      +12     
  Misses      10296    10296

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.

4 participants