👌 Fail and emit warning on filters that do not return a boolean result

[big1hc]

No description provided.

[danwos]

Thanks for the Bugfix 👍

The code itself looks good, but I'm not sure if the check gets performed on the right location.
As we have

result = bool(eval(filter_string, filter_context))`

result should be always True or False.
No matter what eval() returns, it is typecasted to a boolean value.

I think the intention of this fix is to check if the provided filter statement is returning a boolean value or something else.
So for me, the check should be performed on the result of eval() before bool() to be sure the filter statement is working as expected.
Does this cover your use case?

Feel also free to add yourself to the AUTHORS file and maybe add a short bugfix-entry to docs/changelog.rst.

[big1hc]

Hello @danwos
I have change the new logic here. We only raise an error when the result type is a string otherwise convert the result to boolean. because the val() function will return some data types: bool, NoneType, class re.Match (filter is search()), string, None... And I see when it returns string type we can't convert it becomes True/False

[danwos]

Thanks for the update, but we should change the logic for the check to be more bulletproof.

[danwos]

It makes more sense to check here, if the result is not a boolean value, as this is the only allowed return type for a filter.

[big1hc]

I see that when we use the string filter like "search("AAA", title)" -> the result type is re.Match type, this result is not a boolean but it also valid, so that If we only catch the result as a boolean, the filter with search() will not work
I have convert result to boolean when it is returned. We will cover all data type except a string

[danwos]

Mhh then this is a bug for search(). And luckily search() returns None, which gets transformed to False, or it returns a re.Match object, which is always True.
So we should fix our search() implementation as well and I would stay with my opinion, that we should only allow Boolean returns values for eval().

Checking for string only solves some specific problems, but is not a generic solution, which detects all faulty filter strings.

[big1hc]

@danwos from your idea, I have implemented to catch search() filter that transformed to True if the result return a re.Match object otherwise we will check the data type as boolean or not

[danwos]

I think we should do this not here, but in the search function:
https://github.com/useblocks/sphinx-needs/blob/master/sphinx_needs/filter_common.py#L303

I would add something like this to filter_common.py:

def need_search(*args, **kwargs)
   return bool(re.search(*args, **kwargs))

... some other code

 filter_context["search"] = need_search

Then the result of search() gets transformed when used.
And we can save the check for re.Match here, as we should always get boolean-values back.

Sorry for making your PR a little more complex, but you found a new bug with it and for a generic solution for your problem we need to fix this as well.

[big1hc]

I have added the function as your mention to transform the result of search()
I also want a best solution, no problem or inconvenience for me

[danwos]

Looks good, jsut add changelog line to the not released version.

[chrisjsewell]

Thanks!

[big1hc]

Looks good, jsut add changelog line to the not released version.

I added my change to version 1.4.0

[r-o-b-e-r-t-o]

@danwos Can this PR be merged?

[big1hc]

@danwos the pull request is up-to-date now

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.90%. Comparing base (cf5598f) to head (c6c7a54).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #964   +/-   ##
=======================================
  Coverage   85.90%   85.90%           
=======================================
  Files          56       56           
  Lines        6511     6515    +4     
=======================================
+ Hits         5593     5597    +4     
  Misses        918      918           

Flag	Coverage Δ
pytests	85.90% <100.00%> (+<0.01%)	⬆️

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.

[chrisjsewell]

Will fix the failing doc build, and I want to add a test, after #1128 😄

[chrisjsewell]

Ok this is ready to merge, but ...

I have changed the title from a bug fix, to simply a change, since I don't think this fixes a bug, it simply changes sphinx-needs behaviour:
Before, sphinx-needs allowed any truthy/falsy values to be returned from filters, now, it only allows boolean.
I can see how this could stop users from making mistakes with their filters,
although I do fear this will be a breaking change for some users, as it was in our own internal documentation (although at least with #1128 it will now be easier to locate the issue).

@danwos can you just confirm you are happy with this breaking change, then happy to merge (but if you merge it, please do with a commit message, that mentions the breaking nature of the change 😄)