-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat!: add transformer to filter arguments. #374
feat!: add transformer to filter arguments. #374
Conversation
Thanks for the pull request, @andrey-canon! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
This version updates the filters signature in order to support transformer arguments openedx/event-routing-backends#374
Hi @openedx/arch-bom! Is this something you could please review / merge for us? |
@andrey-canon: Is this just building on work from https://github.com/openedx/event-routing-backends/blob/master/CHANGELOG.rst#710, and should it have the same reviewers? Also, reminder to use 8.0.0 if this is a breaking change. Also, do you intend to fix anything using the older version if there are any? |
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.
@andrey-canon Thanks for creating this PR. Overall looks good. Could you please update the docstrings to reflect addition of transformer
in arguments e.g docstring of run_filter method.
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.
Please see my earlier comment and update the CHANGELOG as a minimum, as well as the version if you plan on releasing this change. Thank you.
This improves the filter usability by adding the transformer to the filter arguments, basically this allows the filter client to access to extra data given by the transformer instance
e71a470
to
f59859a
Compare
This doesn't fix anything this just complements the current implementation, since after using the current feature I realized that was very hard to get some basic information in the external filters e.g event name by adding this transformer the data can be extracted easily from the transformer |
CHANGELOG.rst
Outdated
[8.0.0] | ||
~~~~~~~ | ||
|
||
* Add transformer argument to openedx dynamic filter. |
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.
* Add transformer argument to openedx dynamic filter. | |
* **BREAKING CHANGE**: Add transformer argument to openedx dynamic filter. |
@andrey-canon: Thanks for the version and changelog updates.
In the description, you had written:
I was asking if there were current filters that will be broken and need to be updated to work with this version? |
The filter is an extension point that anyone can create so I'm not aware if someone created that, for sure the community doesn't have any filters since the use the default behavior, since this is very recent probably I'm the only one that has implemented a filter for this library |
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.
Thanks for the update @andrey-canon.
[inform] See https://discuss.openedx.org/t/readthedocs-build-failures-missing-config-or-invalid-build-os/11871
Is this something you can merge, or will you need help with that?
@andrey-canon: See this comment on the cookiecutter update: openedx/edx-cookiecutters#419 (comment). It may have issues. |
f93e46a
to
637812e
Compare
@robrap I included the original fix however this repository has a lot of warnings that's why I had to remove the |
Great. Thanks @andrey-canon. Who will be merging this? And releasing? I'll be out the rest of the week. |
@Ian2012 could you help us with this ? |
@andrey-canon 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description:
This improves the filter usability by adding the transformer to the filter arguments, basically this allows the filter client to access to extra data given by the transformer instance.
Current implementation just pass as argument the result of the decorated method, for example, the activity returned by this method however that activity or another result has limited information, for this example the filler would have something like
In this case, if someone wants to get more data, for example, event_name, it's not possible, so adding the transformer instance allows to the person who is developing the filter to access instance methods and attributes like transformer.get_data() or transformer.event
Testing instructions:
This works like another openedx-filter https://github.com/openedx/openedx-filters/
Note
This is a breaking change since current filters won't work with this version
Merge checklist:
Post merge:
finished.