#947 filter_warning option to replace default "No needs passed the filters" text #1093

kreuzberger · 2024-01-19T21:29:55Z

Add option :filter_warning: to directives (e.g. needtable) to show no text or given text instead of the default text.

codecov · 2024-01-19T21:33:56Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c27bc64) 79.27% compared to head (ee8f6d2) 83.19%.

Files	Patch %	Lines
sphinx_needs/directives/needextract.py	0.00%	1 Missing ⚠️
sphinx_needs/directives/needfilter.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1093      +/-   ##
==========================================
+ Coverage   79.27%   83.19%   +3.92%     
==========================================
  Files          56       56              
  Lines        6460     6456       -4     
==========================================
+ Hits         5121     5371     +250     
+ Misses       1339     1085     -254

Flag	Coverage Δ
pytests	`83.19% <96.61%> (+3.92%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danwos

Thanks for the PR. 👍
Technical realization looks good.

I think the dictionary operations can be shortened. I left 1-2 comments, but it affects all changes for the different directives.

danwos · 2024-01-22T05:48:11Z

sphinx_needs/directives/needextract.py

-            content.append(no_needs_found_paragraph())
+            content.append(
+                no_needs_found_paragraph(
+                    current_needextract["filter_warning"] if "filter_warning" in current_needextract else None


I guess this can be shorten to:

current_needextrac.get("filter_warning", None)

danwos · 2024-01-22T05:51:03Z

sphinx_needs/directives/needfilter.py

@@ -228,7 +228,11 @@ def process_needfilters(
            content.append(puml_node)

        if len(content) == 0:
-            nothing_found = "No needs passed the filters"
+            nothing_found = (


This we could write like

nothing_found = current_needfilter.get('filter_warning', 'No needs passed the filters')

danwos · 2024-01-22T05:55:19Z

tests/doc_test/filter_doc/filter_no_needs.rst

+
+.. needtable::
+   :filter: ("7" in tags)
+   :filter_warning: 


Would add here a positive filter result as well, to be sure the text is not printed if needs got found.

kreuzberger · 2024-01-22T06:17:51Z

Hi! Just your suggestions will be integrated cause they provide for sure a more pythonic solution.
Just 2 additional questions:

the lint gives error due to some apply method i do not know nothing about / unchanged code by my pr. How to solve this?
Technically some directives (e.g. needtables) seem to give this warning message or the filter_warning text, but some directives (e.g. needlist) seem to remove the whole node / content even if the code for the warning message is active? Is this right? intended?

danwos · 2024-01-22T06:39:39Z

Regarding the linter problem, this may happen if mypy or other linter-libs got an update and introduced new checks.

However, you can deactivate the specific test in pyproject.toml (untested!):

[[tool.mypy.overrides]]
module = [
  "sphinx_needs.directives.needextract",
]
disable_error_code = "no-untyped-call"

Regarding needlists, I guess this is a bug or not updated behavior.
For me, writing the "no needs found"-text for all directives should be the common rule.
So if you like, you can fix it in this PR ;)

Thanks for the invested time!! 🙏

…rectives

kreuzberger · 2024-01-22T10:39:44Z

So, made some fixes:

add you hints regarding code style
fix the mypy issue regarding your comments
fix different handling of filtered data implementations in most directives

Changed behaviour:

all directives use the common method directives.utils.no_needs_found_paragraph
If no needs are found due to filter, the default or filter warning message is shown.
This includes that if the warning message is shown the "empty" content is not shown!! This affects:
Tables: no more empty tables shown
Lists: show filter message in the above cases (was not shown before)
needgantt: remove empty figure nodes.
needsequence: remove figure nodes if 0 == messages and just one participant.

Added more tests, including a good case there needs are found and filtered message should not be shown.
filter_no_needs.pdf

kreuzberger · 2024-01-22T11:00:43Z

The checks failed due to upload error to codecov...

kreuzberger · 2024-01-22T11:17:37Z

Just one more questions:
If i run the tests locally, (e.g. nox -s "tests-3.10(sphinx='7.0')" ) i always get an error in this test:
tests/test_sn_collapse_button.py FF [ 90%]

The error message shown is

Timing measurement results (JSON) stored under /tmp/sn_test_build_data/pgkTg3kpnD/variant_doc/_build/html/debug_measurement.json
Timing measurement report (HTML) stored under /tmp/sn_test_build_data/pgkTg3kpnD/variant_doc/_build/html/debug_measurement.html
No version of Cypress is installed in: ~/.cache/Cypress/13.6.3/Cypress

Please reinstall Cypress by running: cypress install

----------

Cypress executable not found at: ~/.cache/Cypress/13.6.3/Cypress/Cypress

Is there a specific problem with the pyvenv/nox or what could get wrong?

danwos · 2024-01-22T11:39:45Z

Cypress is our frontend JavaScript test framework, to test browser based features like the need-collapse button.
It must be installed by hand.

I guess there is a flag for all frontend tests, so it could be deactivate with the right pytest command.

danwos

Looks good. Thanks for the update and the good tests.

I will work a little bit on the docs and merge it then (hopefully today)

kreuzberger · 2024-01-22T17:24:08Z

Looks good. Thanks for the update and the good tests.

I will work a little bit on the docs and merge it then (hopefully today)

Hi! After some time 2 thinks were upcomming:

No tests implemention for needpie, i think were was no "old" implemenation
I would like to add a class for the filter warning, so that it could be styled (e.g. bold, extra box etc). This could be done in the central function for adding the paragraph.

…r warnings

kreuzberger · 2024-01-23T09:05:10Z

With the latest commit i think everything is now successfully finished. If you have no more comments/issues with the PR, i think it is ready to get merged 😄

danwos

Looks good, thanks a lot for the PR 👍

kreuzberger added 3 commits January 19, 2024 21:21

first running version

922e7d6

add further tests for filter_warning option

36bade2

apply make format

a4bdb63

fix lint warnings / errors

7b46f38

danwos requested changes Jan 22, 2024

View reviewed changes

fix coding style hints and change filtered warnings behaviours for di…

8128d60

…rectives

fix broken test due to copy paste error

8944fcb

non functional commit to trigger checks again

4f8a573

danwos approved these changes Jan 22, 2024

View reviewed changes

kreuzberger added 2 commits January 22, 2024 18:42

add class for need_filter_empty for styling

63b6d48

adding filter for needpie and add doc, tests for css styling of filte…

ee8f6d2

…r warnings

danwos approved these changes Jan 24, 2024

View reviewed changes

danwos merged commit 73b961e into useblocks:master Jan 24, 2024
19 checks passed

kreuzberger deleted the issue_947 branch January 24, 2024 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#947 filter_warning option to replace default "No needs passed the filters" text #1093

#947 filter_warning option to replace default "No needs passed the filters" text #1093

kreuzberger commented Jan 19, 2024

codecov bot commented Jan 19, 2024 •

edited

danwos left a comment

danwos Jan 22, 2024 •

edited

danwos Jan 22, 2024

danwos Jan 22, 2024

kreuzberger commented Jan 22, 2024

danwos commented Jan 22, 2024

kreuzberger commented Jan 22, 2024

kreuzberger commented Jan 22, 2024

kreuzberger commented Jan 22, 2024

danwos commented Jan 22, 2024

danwos left a comment

kreuzberger commented Jan 22, 2024

kreuzberger commented Jan 23, 2024

danwos left a comment

#947 filter_warning option to replace default "No needs passed the filters" text #1093

#947 filter_warning option to replace default "No needs passed the filters" text #1093

Conversation

kreuzberger commented Jan 19, 2024

codecov bot commented Jan 19, 2024 • edited

Codecov Report

danwos left a comment

Choose a reason for hiding this comment

danwos Jan 22, 2024 • edited

Choose a reason for hiding this comment

danwos Jan 22, 2024

Choose a reason for hiding this comment

danwos Jan 22, 2024

Choose a reason for hiding this comment

kreuzberger commented Jan 22, 2024

danwos commented Jan 22, 2024

kreuzberger commented Jan 22, 2024

kreuzberger commented Jan 22, 2024

kreuzberger commented Jan 22, 2024

danwos commented Jan 22, 2024

danwos left a comment

Choose a reason for hiding this comment

kreuzberger commented Jan 22, 2024

kreuzberger commented Jan 23, 2024

danwos left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2024 •

edited

danwos Jan 22, 2024 •

edited