gh-66079: IDLE: Ability to run 3rd party code checkers by taleinat · Pull Request #9802 · python/cpython (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This is based on Saimadhav Heblikar's latest patch on the issues tracker.

I've updated it to work based on current master. Specifically, it is now implemented as an integral part of IDLE rather than as an extension, and the configuration is added to the main config dialog.

I'm putting this up to allow others to review and give feedback before I move towards finalizing this. This is why this is still missing tests and some polish.

https://bugs.python.org/issue21880

Issue: IDLE: Ability to run 3rd party code checkers #66079

Hi @taleinat! I've just started looking at this and it's going to take me some time to go through, but I have one question. ConfigCheckerDialog is commented out in checkers.py and added to configdialog.py. Was there a reason you wanted it there and not in query.py? Thanks!

@@ -111,11 +112,13 @@ def create_widgets(self):
self.fontpage = FontPage(note, self.highpage)
self.keyspage = KeysPage(note)
self.genpage = GenPage(note)
self.chckpage = CheckersPage(note)

Choose a reason for hiding this comment

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

Not sure if chckpage has any advantage over checkpage.

nameLabel = Label(optionsFrame, text='Name of Checker')
commandLabel = Label(optionsFrame, text='Command')
additionalLabel = Label(optionsFrame, text='Additional Args')
currentCallStringLabel = Label(optionsFrame,

Choose a reason for hiding this comment

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

The recent changes to configdialog have taken out the use of Label (and most other widget types) in the variable name. Some of them like list and frame are still used because it clarifies the usage.

Edit: Although label is used in query.py.

self.focus_set()
# frames creation
optionsFrame = Frame(self)
buttonFrame = Frame(self)

Choose a reason for hiding this comment

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

options_frame and buttons_frame would be preferable. I think we're trying to convert IDLE over to PEP8 and away from camel case unless it's a class.

call_string = ''
self.vars['call_string'].set(call_string)

def is_name_ok(self):

Choose a reason for hiding this comment

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

query.py dropped the is and just uses name_ok.

"in {location} in {menu} menu, before running "
"'{checker}' again.".format(checker=checker,
location=location,
menu=menu))

Choose a reason for hiding this comment

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

You should be able to just use an f-string anywhere you've used format.

title = 'Config new checker'
self.wm_title(title)
self.geometry('+%d+%d' % (parent.winfo_rootx() + 30,
parent.winfo_rooty() + 30))

Choose a reason for hiding this comment

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

You can use an f-string here too.

if 'run' in self.menudict:
self.checkers_menu = Menu(self.menubar, tearoff=0)
self.menudict['run'].insert_cascade(3, label='Code Checkers',
menu=self.checkers_menu)

Choose a reason for hiding this comment

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

I think this should be included in mainmenu.py and then the menu populated like helpmenu is in the lines just below this. One thing though -- if there aren't any checkers, the menu item still appears, but hovering shows nothing. I think there should be a default of <None> that's greyed out or something similar so that it looks like the hovering is working.

FWIW, I have an open PR that adds some tests to the editor for the menus. BPO issue 31529. I need to rebase the PR.

Thanks for the review and comments!

I took the existing patch from the issue tracker, got it into working condition and posted here to allow for review and feedback. I'll be sure to address all of the issues you've highlighted if the decision is made that we'd like to have this feature.

ConfigCheckerDialog is commented out in checkers.py and added to configdialog.py. Was there a reason you wanted it there and not in query.py? Thanks!

No good reason, I wouldn't mind moving it there if you and @terryjreedy think that's appropriate.

hauntsaninja changed the title~~bpo-21880: IDLE: Ability to run 3rd party code checkers~~ gh-66079: IDLE: Ability to run 3rd party code checkers

This PR is stale because it has been open for 30 days with no activity.