IDLE: Remove FormatParagraph's width setting from config dialog

[taleinat]

BPO	20577
Nosy	@terryjreedy, @taleinat
Dependencies	bpo-3068: IDLE - Add an extension configuration dialog
Files	taleinat.20140209.IDLE__Remove_format_paragraph_width_from_config_dialog.patch: initial patch (against branch 3.3; changeset 3e62c6c3977e) formatpara - 20577-34.diff: ready to apply formatpara - 20577-34-v2.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/terryjreedy'
closed_at = <Date 2014-12-16.08:37:16.115>
created_at = <Date 2014-02-09.19:28:36.874>
labels = ['expert-IDLE', 'type-feature']
title = "IDLE: Remove FormatParagraph's width setting from config dialog"
updated_at = <Date 2014-12-16.08:37:16.114>
user = 'https://github.com/taleinat'

bugs.python.org fields:

activity = <Date 2014-12-16.08:37:16.114>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2014-12-16.08:37:16.115>
closer = 'terry.reedy'
components = ['IDLE']
creation = <Date 2014-02-09.19:28:36.874>
creator = 'taleinat'
dependencies = ['3068']
files = ['34013', '35562', '35635']
hgrepos = []
issue_num = 20577
keywords = ['patch']
message_count = 20.0
messages = ['210779', '220224', '220229', '220230', '220245', '220281', '220286', '220311', '220570', '220574', '220596', '221931', '221941', '222717', '222719', '222732', '222764', '222765', '222786', '232725']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'taleinat', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue20577'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

[taleinat]

Following up bpo-3068, this is a separate patch which moves the "paragraph reformat width" setting from the "general" tab in IDLE's config dialog to the relevant place in the extensions configuration.

[terryjreedy]

I updated the patch to match a change, since you posted this, from 70 to 72 in .def and 'maxformatwidth' to ' limit' in formatparagraph. I also added a 'guard' value of 72 in case GetOption returns None (as it did before I changed the option name). I checked that there is no instance of 'para' other than in 'param' in configDialog and ran the tests. I view the attached as ready to commit to 3.4 and merge to 3.5. In 2.7, the patch applies cleanly to the files other that configDialog. The problem there is frameEncoding stuff in the context that is not present in 3.4 or the patch. I would just hand edit the 2.7 file to delete the lines again.

Would you like to push this as your first commit?

[taleinat]

Thanks for reviewing this Terry! See two small comments in the review of your patch.

Indeed, I would very much like for this to be my first commit. Thanks for the pointer WRT backporting to 2.7. Just to make sure, I should commit to 3.5 (default) and then backport to 3.4 and 2.7, correct?

[terryjreedy]

Backwards from default was the old, svn way. We hg we merge forward within 3.x. "ready to commit to 3.4 and merge to 3.5.". If you started with 3.5, then you would have to do a null merge of the 3.4 patch into 3.5. It is occasionally done when people decide to backport to maintenacne sometime later, but much nastier.

Feel free to ask any other questions.

Review: PEP-8, yes!.

[taleinat]

What do you mean with the comment regarding PEP-8?

[terryjreedy]

"For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters."

[taleinat]

I'll be damned. 72 it is, then.

What about using the 'default' parameter for GetOption() instead of "or 72"?

[terryjreedy]

'yes!' meant please do so.

[taleinat]

With very minor changes (just use default=72 instead of "or 72" and added a comment explaining why 72 is the default - see attached), I ran the entire test suite (which is a must according to the dev guide). On current default I'm getting consistent failures in test_tk and test_ttk_guionly, which don't seem related. At least I know I managed to include the tests which require a GUI.

Can I continue with committing the patch despite these failures? I guess I should report these failures separately?

[taleinat]

Also, this seems like a small change. Should I mention it in Misc/NEWS?

[terryjreedy]

General rule: If the other failures happen without the patch, then go ahead. This is, unfortunately, all too common. If they only happen with the patch (extremely unlikely for this one), then try to find out why.

Yes, this change should have a NEWS item. However, I would not include it with your first patch. On my machine, hg has trouble properly merging changes to NEWS even when there are no real conflicts. I have a couple of other items for committed patches that I intend to push separately very soon and I would include this one if appropriate.

[terryjreedy]

Since you are busy, I am planning to commit this tomorrow so I can review the extension config patch with this in place.

[taleinat]

I've been waiting to commit this for some time. I'd really like to do this myself, if you don't mind.

I'm just waiting for my SSH key to be added, which is taking a long time since apparently all three people who could do so are traveling and unable to help.