-
-
Notifications
You must be signed in to change notification settings - Fork 426
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
Conversation
@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 Thanks! |
I will take a look at this, thanks. You can update the gold files by cd'ing into tests/gold/html and running |
No need to make a sample report, I'll be running the code. |
It seems good, except if I sort a column in decreasing order, switching pages is sorted by that same column but in increasing order. |
coverage/htmlfiles/coverage_html.js
Outdated
@@ -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") { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
…sence of a recorded column id.
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. |
…' into keep_sorted_columns
…' into keep_sorted_columns
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( |
…' into keep_sorted_columns
Looks great. |
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. |
BTW: How would you like your name to appear in the changelog and contributors files? |
I'd like it to be "Daniel Diniz", thank you for including me! |
This is now released as part of coverage 7.5.1. |
[![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>
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.