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

Documentation issue with python callbacks in FilesCatalog package #254

Closed
ueffel opened this Issue Oct 29, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@ueffel

ueffel commented Oct 29, 2017

In the template file for user defined python callbacks it says:

...
# As an example, here is how a callback function could be implemented.
# This code mimics the default callback implemented in filescatalog.py:

After that there is a template callback. This callback function however does not do the filtering right. That is to say, it doesn't do it like the rest of the package.
I stumbled upon this while write my own callback function and was confused that so many items weren't filtered out and landed in my catalog.
Here is the original bit of code that should do the filtering:

    for filter in profile.filters:
        if filter.match(entry):
            if not filter.inclusive:
                return None
            break

If there is only an inclusive filter, it doesn't exclude items that aren't matching.

My suggestion for an improvement:

   include = len(profile.filters) == 0 # if there are no filters, always include
   for filter in profile.filters:
       if filter.match(entry):
           include = filter.inclusive
           break
   if not include:
      return None
@polyvertex

This comment has been minimized.

Show comment
Hide comment
@polyvertex

polyvertex Oct 29, 2017

Member

Yes I forgot to replicate the modifications made to default_scan_callback in filescatalog.py, to the documentation. The current implementation is actually quite similar to your suggested snippet.

In fact, the way filters is interpreted in that regard has been modified already for the next version as explained in #253 (comment), so the documentation has been updated already.

See the dev branch of the Packages repo for the details. Caution though: globex.py and filefilter.py are missing on purpose

Member

polyvertex commented Oct 29, 2017

Yes I forgot to replicate the modifications made to default_scan_callback in filescatalog.py, to the documentation. The current implementation is actually quite similar to your suggested snippet.

In fact, the way filters is interpreted in that regard has been modified already for the next version as explained in #253 (comment), so the documentation has been updated already.

See the dev branch of the Packages repo for the details. Caution though: globex.py and filefilter.py are missing on purpose

@polyvertex

This comment has been minimized.

Show comment
Hide comment
@polyvertex

polyvertex Oct 31, 2017

Member

I guess I'll close this to cleanup. Do not hesitate to comment though

Member

polyvertex commented Oct 31, 2017

I guess I'll close this to cleanup. Do not hesitate to comment though

@polyvertex polyvertex closed this Oct 31, 2017

@ueffel

This comment has been minimized.

Show comment
Hide comment
@ueffel

ueffel Nov 2, 2017

So wait the template got updated, but it still doesn't do it like you commented:

  • if filters is empty, default is to include the item
  • if filters contains only negative filters, default is to include the item
  • otherwise, default is to exclude the item

Suppose you have one positive (inclusive) and one negative (exclusive) filter, like

...
filters = 
     + ext: .txt
     - ext: .exe

The default behavior would only put *.txt files in the catalog, but behavior implemented in the template not only includes *.txt but everything else, except for *.exe, right? So also *.cs or *.java or anything.

ueffel commented Nov 2, 2017

So wait the template got updated, but it still doesn't do it like you commented:

  • if filters is empty, default is to include the item
  • if filters contains only negative filters, default is to include the item
  • otherwise, default is to exclude the item

Suppose you have one positive (inclusive) and one negative (exclusive) filter, like

...
filters = 
     + ext: .txt
     - ext: .exe

The default behavior would only put *.txt files in the catalog, but behavior implemented in the template not only includes *.txt but everything else, except for *.exe, right? So also *.cs or *.java or anything.

@polyvertex

This comment has been minimized.

Show comment
Hide comment
@polyvertex

polyvertex Nov 2, 2017

Member

With the next release (dev branch), the filters of your example will only include .txt files. So the - ext: .exe filter is superfluous here. As a matter of fact, I fail to see any practical case for such an example (i.e. having negative filters at the end of the (filters list). Could you give any?

The new behavior to be released is meant to be more intuitive than the current one. So if for some reason you believe it could be improved, shoot before I roll the release :)

[...] but it still doesn't do it like you commented

You mean there's a bug in the dev branch? I will double-check.

Member

polyvertex commented Nov 2, 2017

With the next release (dev branch), the filters of your example will only include .txt files. So the - ext: .exe filter is superfluous here. As a matter of fact, I fail to see any practical case for such an example (i.e. having negative filters at the end of the (filters list). Could you give any?

The new behavior to be released is meant to be more intuitive than the current one. So if for some reason you believe it could be improved, shoot before I roll the release :)

[...] but it still doesn't do it like you commented

You mean there's a bug in the dev branch? I will double-check.

@ueffel

This comment has been minimized.

Show comment
Hide comment
@ueffel

ueffel Nov 2, 2017

I didn't check the dev branch. I should learn to read :(
Dev branch does it right I think but it looked a bit messy before your cleanup a few minutes ago.
Well the example was bad. The point was that there is an inclusive and an exclusive filter.

ueffel commented Nov 2, 2017

I didn't check the dev branch. I should learn to read :(
Dev branch does it right I think but it looked a bit messy before your cleanup a few minutes ago.
Well the example was bad. The point was that there is an inclusive and an exclusive filter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment