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] File: Raise and handle Exc. when file bad pickle #2232

Merged
merged 2 commits into from
Apr 21, 2017
Merged

[FIX] File: Raise and handle Exc. when file bad pickle #2232

merged 2 commits into from
Apr 21, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Apr 13, 2017

Issue

https://sentry.io/biolab/orange3/issues/208584693/

Description of changes

Raise TypeError when PickleReader reads a pickle file without a table (and it suppose to be there).

Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented Apr 13, 2017

@codecov-io
Copy link

codecov-io commented Apr 13, 2017

Codecov Report

Merging #2232 into master will decrease coverage by <.01%.
The diff coverage is 79.16%.

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
- Coverage   72.26%   72.26%   -0.01%     
==========================================
  Files         318      318              
  Lines       54856    54864       +8     
==========================================
+ Hits        39644    39649       +5     
- Misses      15212    15215       +3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d61610...852055a. Read the comment docs.

return pickle.load(f)
table = pickle.load(f)
if not isinstance(table, Table):
raise Exception("Not a table.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't raise instances of Exception; Exception is just the base class for (most) other exceptions. I guess that TypeError would be the appropriate type of exception for this.

You can't even catch Exception without pylint complaining (unless silenced).

Copy link
Contributor

Choose a reason for hiding this comment

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

Change the message to "file does not contain a data table". It's more informative and, like most Python exceptions, starts with a lower case and has no period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -195,3 +195,21 @@ def test_check_datetime_disabled(self):
for i in range(4):
vartype_delegate.setEditorData(combo, idx(i))
self.assertEqual(combo.count(), counts[i])

def test_open_bad_pickle(self):
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 not the proper test for this code (and not the proper place for the test function itself). You modified Orange.data.io.PickleReader, so you should test Orange.data.io.PickleReader and not a widget. You should always aim to test as little code as possible. The test should not rely on the way in which a widget that uses the reader handles exceptions.

Second, simplify this test. You don't need temporary files. Instead of writing a temporary pickle file, just patch open and pickle.load to return None. Then call self.assertRaises(TypeError, PickleReader.read, "foo"). You'll do everything in three simple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

jerneju added 2 commits April 14, 2017 09:41
Raise TypeError when PickleReader reads a pickle
file without a table (and it suppose to be there).
https://sentry.io/biolab/orange3/issues/208584693/
@janezd janezd merged commit c0d2199 into biolab:master Apr 21, 2017
@jerneju jerneju deleted the attribute-file branch April 21, 2017 13:20
@jerneju
Copy link
Contributor Author

jerneju commented May 11, 2017

@jerneju
Copy link
Contributor Author

jerneju commented May 16, 2017

@jerneju
Copy link
Contributor Author

jerneju commented Jun 8, 2017

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