Avoid confusion between hook constants and hook strings

[kdmccormick]

Context

Users can add hook callbacks in two ways:

1. hooks.filters.add_item("filter:name", callback)
2. hooks.Filters.FILTER_NAME.add_item(callback)

These two methods look quite similar. I watched this confuse a plugin developer--they were trying to write hooks.filters.add_item("FILTER_NAME", callback). You can see why it'd be hard to notice why there was anything wrong with that line. And, since there is no error reporting when you add a callback to a non-existent filter (like "FILTER_NAME"), they were stuck on this problem for quite a while.

If you subscribe to the Zen of Python, I think we're violating two principles here:

• Errors should never pass silently.
• There should be one-- and preferably only one --obvious way to do it.

Proposed Acceptance Criteria

Rename the Action.get and Filter.get method to Action.define and Filter.define. These methods should be called once-- and exactly once --for every hook. Re-defining a hook would raise an error, as would trying to reference a hook that isn't defined.

Remove the string-based add function (and its variants) from the API interface. API consumer would only be able to add callbacks by referencing the defined Action and Filter constants. In other words, deprecate method (1) in favor of method (2) above.

We may need to do some thinking in order to make this work with template-string-based hooks, but I think it's doable.

[kdmccormick]

@regisb No rush on this one, but it's ready for your opinion when you have the time.

[regisb]

The more general problem here is the high level hooks API. Personally, I'm unhappy with:

• The name of the "consts" module. Maybe "catalog" would be better?
• The fact that we have to juggle between hooks.filters (== tutor.hooks.filters) and hooks.Filters (== tutor.hooks.consts.Filters). Same for actions and contexts. This is confusing.
• The Tutor docs make a poor job at explaining how hooks should be used. Even I have trouble deciding where I should go from this page: https://docs.tutor.overhang.io/reference/api/hooks/index.html
• I agree that we should discourage the use of filters.add. Would a change in documentation be sufficient? If yes, then I'd rather not change the API too much. If not, then I agree with your proposed changes:
  • Introduce a [Filter|Action].create method (isn "create" more conventional than "define"?)
  • Get rid of the add functions -- actually most of the syntactic-sugar API from the filters/actions/contexts modules.
  • As a consequence of the last item, we will be able to get rid of the Filters/Actions static INDEX attributes... which is great! Even better: we'll no longer need the filter/action names. We'll just need to figure out how to implement the clear_all functions. I expect this will considerably simplify how unit tests are run, if done correctly...

(I realise that I may be getting carried along a little too far in the last bullet point... We should probably implement proposed changes only up to the last item)

[kdmccormick]

@regisb great points.

The name of the "consts" module. Maybe "catalog" would be better?

I agree that "catalog" is better than "consts", but I think the best solution is to get rid of the nesting altogether. As you mention there are ways to import anything from the API:

• tutor.hooks.[consts|filters|actions].*
• tutor.hooks.*

The docs are structured to reflect the long form, but all our examples and the cookiecutter use the short form. I think we should choose one form and get rid of the other. In interest of making the API feel concise and preserving backwards compatibility with our published examples, I propose just flattening the entire API into the tutor.hooks import path. If you'd prefer, we could still break up the implementation into _consts.py, _filters.py, etc., but I'd like that to be a detail that only Tutor core developers notice; I wouldn't like it to be apparent in the docs.

If you're worried about breaking backwards compatibility so soon after the V1 release, we could stub out the consts.py/actions.py/filters.py modules so that they still work for now but print warnings.

With that restructuring done, we could achieve this:

I agree that we should discourage the use of filters.add.

by renaming tutor.hooks.filters.add to tutor.hooks._add_filter_callback. Same with _add_action_callback. In fact, I think we should ensure that every function, constant, and class exported by tutor.hooks is named with a leading underscore unless we are happy enough with it that we want it to be part of the V1 API forever.

The Tutor docs make a poor job at explaining how hooks should be used. Even I have trouble deciding where I should go from this page: https://docs.tutor.overhang.io/reference/api/hooks/index.html

Agreed, I will make an issue to improve the hooks API reference docs.

I agree that we should discourage the use of filters.add. Would a change in documentation be sufficient? If yes, then I'd rather not change the API too much. If not, then I agree with your proposed changes:

I don't think just docs would be sufficient, but I think renaming add as I mention above would be sufficient. That said, I do also like your proposed changes, and if compatibility weren't a concern then I would see no reason not to implement them. I think you have a better sense of the impact that breaking plugin API changes would have on the existing plugin ecosystem than I, so I'll defer to you.

[kdmccormick]

@regisb This came up again today, so I'd like to try to come up with a concrete plan here. What do you think of:

1. Collapse the contents of tutor/hooks/*.py into tutor/hooks/__init__.py. The resulting module would be ~1200 lines, which doesn't seem too long to me, especially since all the code is closely related.
   • Several module-level functions would have name conflicts: add, get, etc. Just delete all of these functions.
   • The result of this is that the entire public API would be within the tutor.hooks module.
2. Re-create tutor/hooks/{consts,actions,filters,contexts}.py as wrappers that just call out to __init__.py in order to maintain backwards compatibility. Raise deprecation warnings in these wrappers.
   • In the Q release, remove the wrapper modules, and simplify tutor/hooks/__init__.py into tutor/hooks.py.
3. In the Filter and Action classes, either:
   1. Move forward with your .create idea, or
   2. Rename .get to ._get. Then, re-create .get as a wrapper around ._get that raises a big deprecation warning, and remove the wrapper in the Q release. Thus, Tutor internally would still use the string names for hooks, but they would be hidden to API consumers.
4. In the docs, either hide all deprecated API functions or at least make it clear that they shouldn't be used.

[regisb]

@kdmccormick your plan is sound, but I think it's possible to make more lightweight changes that address the misunderstandings. I created a PR to resolve the confusions. Please let me know what you think.

[regisb]

I believe we can close this now that this PR was merged.

[kdmccormick]

Announced here: https://discuss.openedx.org/t/simplifying-documenting-the-tutor-hooks-api/9258