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: Move Pylint plugin to new API #12160

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

goanpeca
Copy link
Member

@goanpeca goanpeca commented Apr 6, 2020

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Took the opportunity to simplify the buttons so that it is similar to find in files (start/stop are the same button)

pylint

Issue(s) Resolved

Depends on #11741
Fixes #12183

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 Apr 6, 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-11-19 17:26:33 UTC

@goanpeca goanpeca force-pushed the enh/api-pylint branch 3 times, most recently from 8aadfd1 to ba5ebea Compare April 6, 2020 17:57
@goanpeca goanpeca added this to the Sprint April milestone Apr 6, 2020
@goanpeca goanpeca self-assigned this Apr 6, 2020
@goanpeca goanpeca force-pushed the enh/api-pylint branch 4 times, most recently from 8bfc4cc to 82cc8b8 Compare April 7, 2020 02:17
@goanpeca goanpeca changed the title WIP: Move pylint plugin to new API WIP: Move Pylint plugin to new API Apr 7, 2020
@goanpeca goanpeca force-pushed the enh/api-pylint branch 12 times, most recently from 679c0dc to 3eb922f Compare April 12, 2020 04:07
@goanpeca goanpeca changed the title WIP: Move Pylint plugin to new API PR: Move Pylint plugin to new API Apr 12, 2020
@goanpeca goanpeca force-pushed the enh/api-pylint branch 6 times, most recently from 8496338 to a547980 Compare April 14, 2020 03:40
@github-actions
Copy link

github-actions bot commented Sep 1, 2020

Binder 👈 Launch a Binder instance on this branch

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

@goanpeca left a question and also I think this plugin should use the spinner. Besides that LGTM 👍

spyder/plugins/pylint/main_widget.py Outdated Show resolved Hide resolved
@goanpeca
Copy link
Member Author

goanpeca commented Sep 1, 2020

@goanpeca left a question and also I think this plugin should use the spinner. Besides that LGTM 👍

Thanks for the feedback @dalthviz, made the requested fix.

@dalthviz
Copy link
Member

dalthviz commented Sep 2, 2020

@andfoy @steff456 could you give to this PR a manual test on Linux and Mac? Thanks!

@steff456
Copy link
Member

steff456 commented Sep 2, 2020

/show binder

@github-actions
Copy link

github-actions bot commented Sep 2, 2020

Binder 👈 Launch a Binder instance on this branch

@steff456
Copy link
Member

steff456 commented Sep 2, 2020

Hi @goanpeca and @dalthviz,

With @andfoy we manually review this PR and everything is working as expected.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

@goanpeca one last comment, please unify the use of " to enclose strings, there is a mix (as an example the pylint main_widget.py in the setup of the actions) and using just " could improve future readability of the code. Thanks!

Edit: As an example the code of the setup method:

def setup(self, options):
change_history_depth_action = self.create_action(
PylintWidgetActions.ChangeHistory,
text=_("History..."),
tip=_("Set history maximum entries"),
icon=self.create_icon('history'),
triggered=self.change_history_depth,
)
self.code_analysis_action = self.create_action(
PylintWidgetActions.RunCodeAnalysis,
icon_text=_("Analyze"),
text=_("Run code analysis"),
tip=_("Run code analysis"),
icon=self.create_icon('run'),
triggered=lambda: self.sig_start_analysis_requested.emit(),
context=Qt.ApplicationShortcut,
)
self.browse_action = self.create_action(
PylintWidgetActions.BrowseFile,
text=_('Select Python file'),
tip=_('Select Python file'),
icon=self.create_icon('fileopen'),
triggered=self.select_file,

@goanpeca
Copy link
Member Author

goanpeca commented Sep 3, 2020

@goanpeca one last comment, please unify the use of " to enclose strings, there is a mix (as an example the pylint main_widget.py in the setup of the actions) and using just " could improve future readability of the code. Thanks!

Fixed @dalthviz.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

@goanpeca seems like no change regarding the mix of ' and " was added with the rebase, could you check what happen?

spyder/plugins/pylint/main_widget.py Outdated Show resolved Hide resolved
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

@goanpeca a comment pointing were the CIs are failing

"pylint",
"--output-format=text",
"--msg-template="
""{msg_id}:{symbol}:{line:3d},{column}: {msg}"",
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the replace needs some changes here (this is causing the error in the CIs)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will Fix...!

Copy link
Member

Choose a reason for hiding this comment

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

Also, some PEP8 errors raised. I know these kind of things are kind of a pain to change so thanks for the effort 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries @dalthviz, it is how it goes.

Thanks for all the reviewing. You are all doing a great job, and practice makes perfect :-) !

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

@goanpeca checking seems like the selected path in the combobox is not being taken in to account unless you use the Select python file button to update it:

combopylint

Edit: Seems like the Analyze button always triggers the action using the current filename in the Editor

@goanpeca
Copy link
Member Author

goanpeca commented Sep 3, 2020

Edit: Seems like the Analyze button always triggers the action using the current filename in the Editor

Bug!

Will fix.

@goanpeca goanpeca mentioned this pull request Sep 15, 2020
34 tasks
@ccordoba12 ccordoba12 modified the milestones: v5.0alpha2, v5.0alpha3 Nov 12, 2020
@ccordoba12 ccordoba12 assigned dalthviz and unassigned goanpeca Nov 17, 2020
@ccordoba12
Copy link
Member

Hey @dalthviz, is this one ready for merge?

@dalthviz
Copy link
Member

@ccordoba12 I would say that some testing could be nice, otherwise the missing thing is a squash

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.

Thanks @goanpeca for your work on this PR and @dalthviz for your help to finish it.

I also gave this PR some manual testing too (per @dalthviz's request) and it's working as expected.

@ccordoba12 ccordoba12 merged commit 2d57a20 into spyder-ide:master Nov 19, 2020
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.

Move Pylint plugin to use new API
6 participants