Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from Apr 30, 2024

Conversation

devdanzin
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Owner

nedbat commented Apr 26, 2024

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
Copy link
Owner

nedbat commented Apr 26, 2024

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

@nedbat
Copy link
Owner

nedbat commented Apr 27, 2024

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

@@ -81,7 +81,31 @@ function sortColumn(th) {
.forEach(tr => tr.parentElement.appendChild(tr));

// Save the sort order for next time.
localStorage.setItem(coverage.INDEX_SORT_STORAGE, JSON.stringify({column, direction}));
if (th.id !== "function_or_class") {
Copy link
Owner

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@devdanzin
Copy link
Contributor Author

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.

@devdanzin devdanzin marked this pull request as ready for review April 27, 2024 13:09
@nedbat
Copy link
Owner

nedbat commented Apr 30, 2024

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
Copy link
Contributor Author

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

Looks great.

@nedbat
Copy link
Owner

nedbat commented Apr 30, 2024

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 nedbat merged commit 2bb5ef2 into nedbat:master Apr 30, 2024
35 checks passed
@nedbat
Copy link
Owner

nedbat commented May 2, 2024

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

@devdanzin
Copy link
Contributor Author

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

@devdanzin devdanzin deleted the keep_sorted_columns branch May 2, 2024 10:33
@nedbat
Copy link
Owner

nedbat commented May 4, 2024

This is now released as part of coverage 7.5.1.

renovate bot added a commit to allenporter/flux-local that referenced this pull request May 4, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [coverage](https://togithub.com/nedbat/coveragepy) | `==7.5.0` ->
`==7.5.1` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/coverage/7.5.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/coverage/7.5.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/coverage/7.5.0/7.5.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/coverage/7.5.0/7.5.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>nedbat/coveragepy (coverage)</summary>

###
[`v7.5.1`](https://togithub.com/nedbat/coveragepy/blob/HEAD/CHANGES.rst#Version-751--2024-05-04)

[Compare
Source](https://togithub.com/nedbat/coveragepy/compare/7.5.0...7.5.1)

- Fix: a pragma comment on the continuation lines of a multi-line
statement
now excludes the statement and its body, the same as if the pragma is
on the first line. This closes `issue 754`*. The fix was contributed by
    `Daniel Diniz <pull 1773_>`*.

- Fix: very complex source files like `this one <resolvent_lookup_>`\_
could
cause a maximum recursion error when creating an HTML report. This is
now
    fixed, closing `issue 1774`\_.

-   HTML report improvements:

- Support files (JavaScript and CSS) referenced by the HTML report now
have
hashes added to their names to ensure updated files are used instead of
        stale cached copies.

- Missing branch coverage explanations that said "the condition was
never
false" now read "the condition was always true" because it's easier to
        understand.

- Column sort order is remembered better as you move between the index
pages,
        fixing `issue 1766`*.  Thanks, `Daniel Diniz <pull 1768_>`*.

.. \_resolvent_lookup:
https://github.com/sympy/sympy/blob/130950f3e6b3f97fcc17f4599ac08f70fdd2e9d4/sympy/polys/numberfields/resolvent_lookup.py
.. \_issue
754[nedbat/coveragepy#754
.. \_issue
176[nedbat/coveragepy#1766
.. \_pull
17[nedbat/coveragepy#1768
.. \_pull
1[nedbat/coveragepy#1773
.. \_issue
[nedbat/coveragepy#1774

.. \_changes\_7-5-0:

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/allenporter/flux-local).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMzEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjMzMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTML column sorting is inconsistent across index pages
2 participants