I want to follow the safe way to refactor (from a blog post), rather than the hacker way of refactoring 'in place'.
Step 0: Test code to be refactored. Finished in 30993.

Step 1: Copy code to be refactored and modify as needed to isolate it from the working original. For configdialog, add 'class FontPage' below ConfigDialog and copy font/tab methods, now collected together, below that. For the test, copy class FontTest as FontPageTest and IndentTest as IndentOptionTest. The copies should pass as they will still be testing the original code. Commit.

Step 2: Modify code as desired. Modify test copy to test code copy. For FontPage, this will require new tracer manager (bpo-30853) to attach (activate) callbacks so tests will pass. Commit.

Step 3: Switch from using original code to using modified code. For this issue, calling FontPage instead of create_page_font_tab may be enough. Skip original test; modify new test as needed to pass. Commit.

Step 4: Once we are sure that we do not need the original code that has been replaced, delete it. Rerun tests. Commit.

The separate commits will make review easier. Create or update PRs as desired.

PR is for step 1 since VarTrace is needed for step 2.

I may have changed the Font group since the copy was made. So unless merges since are carefully checked, the configdialog part of Step 1 should be redone. I think the latter would be easier.

Since bpo-30853 might be merged tomorrow, I will 'freeze' the font/indent code unless I hear otherwise, or there is an unexpected delay. When this is merged, I will do the same for the general part. And so on.

In Step 2, delete 'Modify code as desired.' I must have forgotten the synchonization problem. We don't want to modify the copy until it can be htested. As soon as the copy *is* changed, the original is no longer a complete backup.

I thought of saying that FontPage should subclass Frame, like we have done before. But TabbedPageSet creates a Frame for each page name. So ConfigDialog, the only user, is expected to pass names, then retrieve the blank frames to fill in. We could change that, but not now unless we really need to. It should be possible to replace the blank frame, but there are at least two references to replace.

For steps 2 and 3, setUpClass will have to create or retrieve a reverence to the page class instance. For page functions, 'dialog' will have to be changed to 'self.page'. For hightlight and key pages, I will write the tests using 'self.page', which would start as a synonym for 'dialog' (set in setUpClass). I might convert GeneralTest before you copy it.

OK, once 30853 is merged, I'll recopy the font code into the class.

On Fri, Jul 28, 2017 at 2:06 AM, Terry J. Reedy <report@bugs.python.org>
wrote:

Terry J. Reedy added the comment:

For steps 2 and 3, setUpClass will have to create or retrieve a reverence
to the page class instance. For page functions, 'dialog' will have to be
changed to 'self.page'. For hightlight and key pages, I will write the
tests using 'self.page', which would start as a synonym for 'dialog' (set
in setUpClass). I might convert GeneralTest before you copy it.

----------

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue31004\>

---

When replying by email, snip the quote.

I will push Notebook when CI passes. We can then do Step 2 for class FontPage(Frame). Consider the font code frozen for now. Pass 'note', not the dialog, as the parent. This makes tab switching work better.

Create the highlight frame first and pass it as a second argument to FontPage. Adding set_samples to load_font_config should solve the initial font problem.

I'm pushing step 2 with an error in the test. It's on the keydown in test_fontlist_key. I just didn't want to hold you up from looking at the rest of it because of one test.

I did run into a bunch of other issues/questions while doing this. I didn't add tracers.attach() to the FontPage class. So, I had to call it in the FontPageTest. Well, I then also added a detach so it wouldn't carry to the other tests and of course all the other tests broke. So, I added setup/teardown of tracers in all the test classes. I don't know if that was the right way to go or if adding tracers.attach to FontPage and then just leaving everything attached would be ok.

Also, I tried to minimize the change from d=dialog to d = self.page, so I kept the name d.

Adding set_samples to load_font_cfg upped the called count to 4 in test_load_font_cfg because the tracers are attached when it's called. I think you mentioned that test needs to be changed with them detached, but I wasn't sure if you wanted that changed now or later.

In FontPage, I tried not to change code yet, so I keep the name parent even though note will be passed in.

I am going to work on this now. Tracers should be on for the duration of the test class except for the load test. d = self.page is what I planned initially.

Argument note is bound to parameter parent and that will remain the name.

The one error I see in PageFont before testing is adding or leaving "frame = Frame(self.parent)". Now that FontPage is a frame, 'frame = self' (we can later replace 'frame' with 'self' as the parent argument in calls that follow).

New changeset 9397e2a by Terry Jan Reedy (csabella) in branch 'master':
bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) (bpo-2905)
9397e2a

New changeset 7582226 by Terry Jan Reedy in branch '3.6':
[3.6] bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) (GH-2905) (bpo-2950)
7582226

At last! On to the next one.

New changeset 9397e2a by Terry Jan Reedy (csabella) in branch 'master':
bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) (bpo-2905)

This change introduced reference leaks in test_idle: see bpo-31130.

By the way, I suggest to not backport refactoring changes to stable branches like Python 3.6. Any refactoring is a risk of regressions. Well, it's just an advice, Terry knows IDLE much better than me ;-) It's up to you to decide to backport or not such change.

Idlelib is a bit exceptional: see PEP-434. Except for one test and the news files, I keep idlelib identical in 3.6 and 3.7. This makes all backports, *including new tests*, trivial and overall reduces the risk of regression (unless IDLE 3.6 were abandoned completely). The risk of regression is why I have only touched 3.6/7 since last Fall, why I am increasingly strict about testing, and why I will manually test everything at least a few days before the next release candidate.