gh-66079: IDLE: Ability to run 3rd party code checkers #9802
Conversation
|
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. |
| @@ -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) | |||
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
No good reason, I wouldn't mind moving it there if you and @terryjreedy think that's appropriate. |
|
This PR is stale because it has been open for 30 days with no activity. |
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