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

PR: Generalize error reporting via signal and simplify widget #13375

Merged
merged 2 commits into from
Jul 28, 2020

Conversation

goanpeca
Copy link
Member

@goanpeca goanpeca commented Jul 24, 2020

Description of Changes

  • This adds error handling using a signal so devs do not need to call the Console Plugin directly (that should not be needed in general)
  • Added documentation to signals in the Plugin and Widgets API
  • Move some methods from the main window into the error report dialog, since there was a lot of back and forth and checks if a parent was the main window. I simplified things, since there was an error hapenning.
  • Move tests from the main window to the report dialog tests (based on the refactor)
  • Adapted a Variable Explorer test so that it passes locally on python 3.6 and OSX

Issue(s) Resolved

Fixes #

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @goanpeca

@pep8speaks
Copy link

pep8speaks commented Jul 24, 2020

Hello @goanpeca! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-28 13:28:26 UTC

@goanpeca goanpeca self-assigned this Jul 24, 2020
@goanpeca goanpeca added this to the Sprint July milestone Jul 24, 2020
@goanpeca goanpeca changed the title WIP: Generilize error reporting via signal and simplify widget PR: Generalize error reporting via signal and simplify widget Jul 24, 2020
@goanpeca goanpeca requested review from ccordoba12 and dalthviz and removed request for ccordoba12 July 24, 2020 20:34
@goanpeca goanpeca marked this pull request as ready for review July 24, 2020 20:34
@goanpeca goanpeca requested a review from ccordoba12 July 24, 2020 20:35
@goanpeca
Copy link
Member Author

@nerohmot, it seems I had not yet included the signal (or I did in another API PR?) anyway, this addresses what we spoke today and also fixed some found issues.

This is ready for review @ccordoba12. @dalthviz if you could also test this that would be great.

Thanks.

@ccordoba12
Copy link
Member

Thanks @goanpeca for this! Although the tests I added last time we changed this should catch errors with this functionality, this is a big change that deserves a bit of manual testing.

@dalthviz, please test that we can still report errors generated by the PyLS and internal errors too. For the first ones, you can put any kind error here

def pyls_completions(config, document, position):
"""Get formatted completions for current code position"""
settings = config.plugin_settings('jedi_completion', document_path=document.path)
code_position = _utils.position_to_jedi_linecolumn(document, position)

(e.g. import foo) and then try to get completions with python bootstrap.py.

For the second ones, I usually put an error here

def show_dependencies(self):
"""Show Spyder's Dependencies dialog box"""
from spyder.widgets.dependencies import DependenciesDialog
dlg = DependenciesDialog(self)
dlg.set_data(dependencies.DEPENDENCIES)
dlg.exec_()

and then try to show the dependencies dialog from the Help menu.

@dalthviz
Copy link
Member

@ccordoba12 @goanpeca checked and in both cases (pyls and internal error) the report dialog pops-up 👍 however when trying to send the issue to Github this is what I'm getting:

image

image

In the cmd used to run spyder I get the following:

WARNING:spyder.widgets.github.backend:Failed to send bug report on Github. response={'code': 404, 'json': {'message': 'Not Found', 'documentation_url': 'https://developer.github.com/v3/issues/#create-an-issue'}}

@goanpeca
Copy link
Member Author

Thanks for checking @dalthviz. Will take a look.

@goanpeca
Copy link
Member Author

goanpeca commented Jul 27, 2020

@dalthviz fixed the issue. Could you test again?

Also, it would be great if you could also test the Report issue under the Help Menu.

Thanks!

@ccordoba12
Copy link
Member

/binder

@github-actions
Copy link

Binder 👈 Launch a Binder instance on this branch

spyder/api/plugins.py Outdated Show resolved Hide resolved
spyder/api/plugins.py Outdated Show resolved Hide resolved
spyder/api/plugins.py Outdated Show resolved Hide resolved
spyder/api/widgets/__init__.py Outdated Show resolved Hide resolved
spyder/api/widgets/__init__.py Outdated Show resolved Hide resolved
spyder/app/tests/test_mainwindow.py Outdated Show resolved Hide resolved
Comment on lines +135 to +138
# Since rowCount for python 3 and 2 varies on differents systems,
# we use a range of values
expected_output_range = list(range(min(row_count), max(row_count) + 1))
assert model.rowCount(model.index(0, 0)) in expected_output_range
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary now that we don't support Python 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

The number of items varies from python version (3.5, 3.6 3.7 3.8). The removal of py2 specific tests is done in the variable explorer migration PR.

spyder/app/mainwindow.py Show resolved Hide resolved
spyder/widgets/reporterror.py Outdated Show resolved Hide resolved
spyder/widgets/reporterror.py Outdated Show resolved Hide resolved
@goanpeca goanpeca requested a review from ccordoba12 July 28, 2020 13:28
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

I checked this on Binder for both internal errors and issue reports and things are working as expected.

Thanks @goanpeca!

@ccordoba12 ccordoba12 merged commit fe48aa6 into spyder-ide:master Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants