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] Report: handle PermissionError when trying to save #2225

Merged
merged 1 commit into from
Apr 28, 2017
Merged

[FIX] Report: handle PermissionError when trying to save #2225

merged 1 commit into from
Apr 28, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Apr 12, 2017

Issue

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

Description of changes

Now shows a windows with error message and logs error as well.

Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented Apr 12, 2017

@codecov-io
Copy link

codecov-io commented Apr 12, 2017

Codecov Report

Merging #2225 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2225   +/-   ##
=======================================
  Coverage   72.37%   72.37%           
=======================================
  Files         319      319           
  Lines       55014    55014           
=======================================
  Hits        39819    39819           
  Misses      15195    15195

@@ -162,13 +162,13 @@ def _setup_ui_(self):

class PyBridge(QObject):
@pyqtSlot(str)
def _select_item(myself, item_id):
def _select_item(self, item_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this argument should have stayed myself; using self here shadows the self from the outer scope.

See the line below: do instances of QObject have an attribute table_model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shame on me. I blindly followed lint's complaints.

item = self.table_model.get_item_by_id(item_id)
self.table.selectRow(self.table_model.indexFromItem(item).row())
self._change_selected_item(item)

@pyqtSlot(str, str)
def _add_comment(myself, item_id, value):
def _add_comment(self, item_id, value):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@@ -337,14 +340,14 @@ def open_report(self):

try:
report = self.load(filename)
except (IOError, AttributeError) as e:
except (IOError, AttributeError):
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to open a file with extension .report, but which was not actually a report file. It crashes with UnpicklingError. Perhaps you should add it to list, or even catch the general Exception here.

parent=self)
self.tr("Could not load an Orange Report file"),
title=self.tr("Error"),
informative_text=self.tr("An unexpected error occurred "
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unexpected (all errors are unexpected) and probably also the article, hence "Error occurred while loading '%s'.

self.tr("Could not load an Orange Report file"),
title=self.tr("Error"),
informative_text=self.tr("An unexpected error occurred "
"while loading '%s'.") % filename,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use formatting with {} and format, not the C-style %. I would say the the old-style is permitted in Orange only for consistency in the modules that (still) use the old style.

@janezd janezd merged commit e815184 into biolab:master Apr 28, 2017
@jerneju jerneju deleted the permission-report branch May 3, 2017 07:16
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