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] The Guardian, Twitter, NYT: Error credentials #253

Closed
wants to merge 3 commits into from
Closed

[FIX] The Guardian, Twitter, NYT: Error credentials #253

wants to merge 3 commits into from

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented May 29, 2017

Issue
Description of changes

try except block

Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented May 29, 2017

@jerneju
Copy link
Contributor Author

jerneju commented May 29, 2017

@codecov-io
Copy link

codecov-io commented May 29, 2017

Codecov Report

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

@@           Coverage Diff           @@
##           master     #253   +/-   ##
=======================================
  Coverage   93.82%   93.82%           
=======================================
  Files          32       32           
  Lines        1603     1603           
  Branches      295      295           
=======================================
  Hits         1504     1504           
  Misses         60       60           
  Partials       39       39

@jerneju
Copy link
Contributor Author

jerneju commented May 29, 2017

@nikicc
Copy link
Contributor

nikicc commented May 29, 2017

@jerneju should we maybe fix CredentialManager.__getattr__ to catch the error and then return an empty string?

This is fixing retrieving from CredentialManager. What about storing into it? Can the same thing happen there?

@jerneju
Copy link
Contributor Author

jerneju commented May 29, 2017

@nikicc if we do not plan to show an error message in the future, then fixing CredentialManager is way better. Is there any widget where user really should be informed about this error?

@jerneju
Copy link
Contributor Author

jerneju commented May 30, 2017

@nikicc I have finally done the test. It does raise an error when trying to store it.

@nikicc
Copy link
Contributor

nikicc commented May 31, 2017

This is probably blocked on biolab/orange3#2354 right? If so, we should probably bump the Orange version in requirements?

Copy link
Contributor

@nikicc nikicc left a comment

Choose a reason for hiding this comment

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

These tests cannot run until the new version of Orange is out, since on Text we install Orange through pip. We should at least add something like:

    @unittest.skipIf(LooseVersion(Orange.__version__) < LooseVersion('3.4.3'),
                     'Not supported in versions of Orange below 3.4.3')

or probably even better: move the test to the core Orange.

@jerneju
Copy link
Contributor Author

jerneju commented Jun 1, 2017

Closed in favor of biolab/orange3#2354 .

@jerneju jerneju closed this Jun 1, 2017
@jerneju jerneju deleted the error-credentials branch June 1, 2017 08:03
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