-
Notifications
You must be signed in to change notification settings - Fork 17
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
docs: add newest docs for openedx-filters #64
Conversation
Thanks for the pull request, @mariajgrimaldi! As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion. |
e2c2ef4
to
8bad482
Compare
81ee4f5
to
d5bb00c
Compare
8bad482
to
4fe2795
Compare
Hey @mariajgrimaldi :) Just checking to see if you plan to pursue this one? |
@mariajgrimaldi I looked through the content you already put in here and it was very valuable, I'd love for you or other filters users to flesh out the rest of this tree that you built. I went ahead and wired everything together so the docs build is green. Are there other things we can do to help with this. @openedx/arch-bom do you have any knowledge in this area of extensibility that you could use to help build out some of these docs? |
@feanil thank you so much! I will write at least the minimum info needed for a 1st version so we can deploy it and iterate over it! I already blocked a slot in my calendar to do it. I'll let you know once I'm done so you folks can review it! |
44d8950
to
7e025ad
Compare
note: we can add specifications about filters associated with the template rendering process, which are a bit different from the usual filters. We can also add a list of filters, like the index from edx-platform, but with descriptions and possible use cases. |
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.
@mariajgrimaldi changes look great! Lots of awesome details, I have a few suggestions to add clarity but overall this looks great!
docs/how-tos/create-new-filter.rst
Outdated
- The ``run_filter`` class method calls the ``run_pipeline`` method, which is the method that executes the filter's logic. This method is defined in the ``OpenEdxPublicFilter`` class, which is the base class for all the filters in the library. This method returns a dictionary with the following structure: | ||
|
||
.. code-block:: python | ||
|
||
{ | ||
"form_data": form_data, | ||
} | ||
|
||
Where ``form_data`` is the same set of parameters that the filter receives, which is the accumulated output for the filter's pipeline. This is how ``run_filter`` should always look like. |
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.
This bullet point is a bit confusing. I think what you're saying is that for this specific filter, the run_pipeline
method will return the dictionary with form_data
in it. But in general what is return is up to the specific filter type? Is that right?
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.
Yes! That's right. I'm adding a clarifying example.
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.
docs/how-tos/using-filters.rst
Outdated
1. The filter step must be a subclass of ``PipelineStep``. | ||
|
||
2. The ``run_filter`` signature must match the filters definition, eg., | ||
the previous step matches the method's definition in CertificateCreationRequested. |
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.
suggestion: It might be useful to put a copy of the filter definition here so that readers can see how the signature in the PipelineStep maps to the signature of the run_filter
in the filter definition. Because when you say the filters definition
it's not clear if you mean the signature of run_pipeline
or of run_filter
in the filter definition.
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.
I rephrased it and added the filter definition: https://github.com/openedx/openedx-filters/pull/64/files#diff-2f00ad15852db823832ebd7a824f9a50eb845116051e7e9c64e99aa0721db8d5R41-R81
Hi @mariajgrimaldi - just following up on this! |
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.
@mariajgrimaldi this is looking great, I had a few small suggestions but then the how-tos for creating and using filters seem good enough to land. I see you have a couple of quickstarts that are still in progress, do you want to do those in a follow-up PR so we can land the pieces that already seem like they're nearly done.
docs/concepts/glossary.rst
Outdated
Filter signature | ||
---------------- | ||
|
||
It's the signature of the `run_filter` method. It defines the input and output of the filter. The input is a dictionary with the data to be processed and the output is a dictionary with the processed data. |
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.
Since both the PipelineStep
and Open edX Filter
class implement a run_filter
function which do you mean here? Do both of them have the same signature, it would be good to clarify that in this glossary item.
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.
Mmm I'll better rephrase this then! Thanks
@@ -0,0 +1,11 @@ | |||
Using Open edX Filters in the LMS service | |||
========================================= |
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.
This document should follow standard heading order as defined here: https://docs.openedx.org/en/latest/documentors/references/quick_reference_rst.html#headings
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! I'll change it
Also drop the changelog from the long description. The way it is right now, that will never succeed the strict checks becaause of the way setuptools currently parses the RST. It generates implicit labels for all headings by default and then it complains when there are duplicates. This will always happen with changelogs because the sub-headings under a changelog entry will always be one of a small set of values (added, fixed, etc.)
b0c9830
to
20d484a
Compare
Thanks for the pull request, @mariajgrimaldi! 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. |
@feanil: last changes! Let me know what you think |
Looks great! Thanks for all the hard work on this @mariajgrimaldi ! |
I'll be merging this now! Thank you folks! |
@mariajgrimaldi 🎉 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 PR adds the latest documentation for Open edX Filters usage.