Make HTML column sorting consistent across index pages (fix #1766)

[devdanzin]

This is a rough sketch of a fix for #1766, by storing column header ids instead of indexes to remember sorting choice.

This PR also presents a solution for the issue that there's an extra column in the Functions and Classes pages. We record the last column used for sorting in the index page, plus a flag that says whether the extra column was used in either of those two pages. If it was, sort by it the next time around.

Since the code quality is poor, this is offered as a draft so we can discuss the design and improve on it.

Fixes #1766.

[devdanzin]

@nedbat When you have time, please let me know how you'd like me to proceed here, no hurry.

Should I publish a coverage HTML report with the changed code to make testing it easier?

Can you check that the sorting behavior when changing pages matches your expectations/specifications? And if it doesn't, what should be done differently?

I was thinking we could first validate the proposed behavior, then I'd work on improving the code. But I'm open to making the code neater before you take a look, if that'd be better.

Should I fix the failing tests by updating the gold HTML files, or would you rather take a look at the index.html changes first?

Thanks!

[nedbat]

I will take a look at this, thanks.

You can update the gold files by cd'ing into tests/gold/html and running make update-gold and make update-support.

[nedbat]

No need to make a sample report, I'll be running the code.

[nedbat]

It seems good, except if I sort a column in decreasing order, switching pages is sorted by that same column but in increasing order.

[nedbat]

I don't want to use function_or_class as the id. I've been careful throughout the function/class implementation to generalize to "region", and in the reports I simply call it "column2" (though now that I type this, "region" would have been better).

[devdanzin]

I've replaced it with "region" and it reads well. Let me know if you'd rather have "column2" instead.

[devdanzin]

I sort a column in decreasing order, switching pages is sorted by that same column but in increasing order.

Good catch, I introduced this bug by loading the saved direction, then recording it instead of the current direction. Fixed.

That led me to realize there's a behavior choice to be made.

When the function page is sorted by function, clicking on a different column header in the index page will sort the function page by that column. However, what should happen when the same header is clicked in the index page (only changing direction)?

Current code ignores that click and keeps the function page sorted by function. We could change the function page sort column to the clicked one instead, meaning a change in direction counts as a change in sort column.

What should we do there? Either way is easy to implement, it's just that it isn't obvious to me which would be better.

[nedbat]

I think I like this as a way to fix the scrubbing of Windows-generated gold files:

diff --git a/tests/test_html.py b/tests/test_html.py
index c4f7870d..1a8c71f9 100644
--- a/tests/test_html.py
+++ b/tests/test_html.py
@@ -709,14 +709,9 @@ def compare_html(
         (filepath_to_regex(abs_file(os.getcwd())), 'TEST_TMPDIR'),
         (filepath_to_regex(flat_rootname(str(abs_file(os.getcwd())))), '_TEST_TMPDIR'),
         (r'/private/var/[\w/]+/pytest-of-\w+/pytest-\d+/(popen-gw\d+/)?t\d+', 'TEST_TMPDIR'),
+        # If the gold files were created on Windows, we need to scrub Windows paths also:
+        (r'[A-Z]:\\Users\\[\w\\]+\\pytest-of-\w+\\pytest-\d+\\(popen-gw\d+\\)?t\d+', 'TEST_TMPDIR'),
     ]
-    if env.WINDOWS:
-        # For file paths...
-        scrubs += [
-            (r'[A-Z]:\\Users\\[\w\\]+\\pytest-of-\w+\\pytest-\d+\\(popen-gw\d+\\)?t\d+',
-             'TEST_TMPDIR')
-        ]
-        scrubs += [(r"\\", "/")]
     if extra_scrubs:
         scrubs += extra_scrubs
     compare(expected, actual, file_pattern="*.html", scrubs=scrubs)
@@ -990,7 +985,8 @@ def test_other(self) -> None:
         compare_html(
             gold_path("html/other"), "out/other",
             extra_scrubs=[
-                (r'href="z_[0-9a-z]{16}_', 'href="_TEST_TMPDIR_othersrc_'),
+                (r'href="z_[0-9a-z]{16}_other_', 'href="_TEST_TMPDIR_other_othersrc_'),
+                (r'TEST_TMPDIR\\othersrc\\other.py', 'TEST_TMPDIR/othersrc/other.py'),
             ],
         )
         contains(

[devdanzin]

I think I like this as a way to fix the scrubbing of Windows-generated gold files:

Looks great.

[nedbat]

Thanks, if there's more to do, it can be a new pull request. I think the "hide covered" checkbox and filter text should probably also be remembered, for example.

[nedbat]

BTW: How would you like your name to appear in the changelog and contributors files?

[devdanzin]

I'd like it to be "Daniel Diniz", thank you for including me!

[nedbat]

This is now released as part of coverage 7.5.1.