exporters icon indicating copy to clipboard operation
exporters copied to clipboard

Add Multiple filters

Open raphapassini opened this issue 8 years ago • 5 comments

raphapassini avatar Jul 27 '16 22:07 raphapassini

First of all thank you for the review @eliasdorneles, I think it makes a lot of sense. I'll work on a new implementation, did you have time to check the tests? Do you think they cover enough what we expected this new Filter to do?

raphapassini avatar Jul 28 '16 16:07 raphapassini

Hello @eliasdorneles please take a look at my last commit - https://github.com/scrapinghub/exporters/pull/325/commits/98f3f698d4306974a95b6deb1bce7e846b901205, I changed the implementation in the way you suggest in your last comment (https://github.com/scrapinghub/exporters/pull/325#discussion-diff-72542313)

raphapassini avatar Aug 01 '16 17:08 raphapassini

What has changed in the last commit?

  • Changed the implementation to support nested filters
  • Throw a ValueError when try to load invalid filters.
  • Update tests to cover nested filters

raphapassini avatar Aug 02 '16 15:08 raphapassini

@eliasdorneles please, take a look in my last update when you have time :)

raphapassini avatar Aug 04 '16 20:08 raphapassini

@raphapassini After reviewing this a bit there are some changes still needed in order for this to work. The code looks fine. I am a little confused on some of the tests. Are we testing for different types of filters together here such as key_value_filters and pythonexp_filter rather than 2 or 3 that are the same?

Here are some items which will need to implemented for this to actually work:

You will need to add the MultipleFilter to this section https://github.com/raphapassini/exporters/blob/737785b86b2fa2d8e66a97aa4eb1d0baadd721a1/exporters/filters/init.py

You will also need to add some code to handle processing the filters section from the config in order for it to be used. Check https://github.com/raphapassini/exporters/blob/737785b86b2fa2d8e66a97aa4eb1d0baadd721a1/exporters/exporter_config.py#L23 how it is done for the filter section. There will be a few other spots in that file that will probably require some changes in order for this module to get loaded. This function https://github.com/raphapassini/exporters/blob/737785b86b2fa2d8e66a97aa4eb1d0baadd721a1/exporters/exporter_config.py#L48 will need to be modified to handle the list of filters rather than just one.

Once these are done then I think we can do some real tests to see if it works with real configurations.

Thanks!

tsrdatatech avatar Nov 17 '16 21:11 tsrdatatech