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] Save Data: Reset writer upon changing the extension #3437

Merged
merged 1 commit into from
Dec 4, 2018

Conversation

ajdapretnar
Copy link
Contributor

Issue

Save Data doesn't change the writer when the user changes the extension.

Description of changes

Upon changing the extension, writer is set accordingly.

Includes
  • Code changes
  • Tests
  • Documentation

@codecov
Copy link

codecov bot commented Dec 3, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3437      +/-   ##
==========================================
+ Coverage   83.25%   83.25%   +<.01%     
==========================================
  Files         365      365              
  Lines       64249    64247       -2     
==========================================
+ Hits        53489    53490       +1     
+ Misses      10760    10757       -3

@janezd
Copy link
Contributor

janezd commented Dec 3, 2018

What about eliminating attribute self.writer, and calling get_writer_selected() at the two places when the widget needs the writer? There is no need to store data that can be trivially computed from the data you already have.

It's preferable to have a single source of truth whenever possible and practical. The bug that you're fixing here occurred because there were two and they were not properly synchronized.

@ajdapretnar
Copy link
Contributor Author

@janezd Fixed as suggested. Could you perhaps give it another look and merge, if everything is ok. #3306 depends on this.

@janezd
Copy link
Contributor

janezd commented Dec 4, 2018

I think that tests shouldn't call update_ext because the widget doesn't call it either.

I thus removed this and the related redundant change of method name. This simplifies the PR so that it fixes only the problem with the writer.

@lanzagar, can you quickly check that it's still OK? Changes are now minimal.

@lanzagar lanzagar changed the title OWSave: Reset writer upon changing the extension [FIX] Save Data: Reset writer upon changing the extension Dec 4, 2018
@lanzagar lanzagar added this to the 3.19 milestone Dec 4, 2018
@lanzagar lanzagar merged commit 6e7ead1 into biolab:master Dec 4, 2018
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