Skip to content
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

Minimal API to enable/disable built-in plugins #229

Closed
lemon24 opened this issue Mar 22, 2021 · 2 comments
Closed

Minimal API to enable/disable built-in plugins #229

lemon24 opened this issue Mar 22, 2021 · 2 comments

Comments

@lemon24
Copy link
Owner

lemon24 commented Mar 22, 2021

With #226, it's become obvious that some plugins should be enabled by default.

There's also reader._config.make_reader_from_config(); ideally, make_reader() should tend toward it.

Important: This is not to make public hooks etc. (#80); the only promise we make is: this is how you enable/disable built-in plugins, and these are the built-in plugins.

Additionally, it would probably nice to allow callable-style plugins (a la Mistune v2); here, the promise we make is: given this list of callables, we'll call them with the instantiated reader before returning it.

Candidates for the status of built-in:

  • ua_fallback (enabled)
  • enclosure_dedupe (disabled, because it changes returned data)
  • feed_entry_dedupe (disabled, because it changes data, and the duplicate detection logic may be too strict)
  • regex_mark_as_read? (disabled); reserves a specific feed metadata key for reader, we may need to revisit Reserve resources for internal use #186
@lemon24 lemon24 changed the title Minimial API to enable/disable "built-in" plugins Minimal API to enable/disable built-in plugins Mar 22, 2021
@lemon24
Copy link
Owner Author

lemon24 commented Mar 22, 2021

Case study: Mistune v2

markdown = mistune.create_markdown(plugins=['strikethrough'])

renderer = mistune.HTMLRenderer()
markdown = mistune.Markdown(renderer, plugins=[plugin_strikethrough])

markdown = mistune.create_markdown(plugins=[DirectiveToc(...)])

The string version uses plugins from a hand-written dict of "built-in" plugins.

On the second strikethrough version, the renderer is required by the plugin (because it needs to do stuff to it); I assume create_markdown() adds it automatically.

Case study: Flask extensions

db = SQLAlchemy(app, ...)

db = SQLAlchemy(...)
db.init_app(app)

Unlike Mistune:

  • the user initializes the extensions
  • the init_app() version can be used to initialize multiple apps with the same extension; it is required that the extension does not store a reference to the app in this case

Case study: make_reader_from_config()

reader = make_reader_from_config(..., plugins=[
    'reader._plugins.regex_mark_as_read:regex_mark_as_read',
    reader._plugins.ua_fallback.init,
])

Like Mistune, it takes either a string or a callable; unlike Mistune, the string is an import path, not a name.


It should be possible to change a make_reader() call to make_reader_from_config() without changing anything else.

If we use plugin names in make_reader(), we risk shadowing other packages; put another way, we need to be able to distinguish between plugin names and import paths from the start. Possible ways to deal with this:

  • namespace built-in plugins: reader.ua_fallback (anything starting with reader. is a built-in plugin)
    • don't like that it looks like an import path
  • require import paths to have the module:object form
    • somewhat unfriendly, see the regex_mark_as_read duplication above (OTOH, if the callable was called init, it'd be much shorter; newer plugins follow this convention; also, our current plugin-loading code requires this form)
    • alternatively, make module: expand to module:init
  • make built-in plugins start with a dot: .ua_fallback actually expands to the import path reader.plugins.ua_fallback

It seems we have a trade-off between:

  • making it easier for people to use built-in plugins: ua_fallback, thirdparty:init
  • making it easier for people to use third-party plugins: reader.ua_fallback, thirdparty (where the plugin loader looks for init by convention)
  • getting both, but having built-in plugins be a bit confusing: .ua_fallback, thirdparty

Update: A one-user focus group concluded import paths everywhere is better:

reader = make_reader('db.sqlite', plugins=[
    'reader.plugins.ua_fallback',
    'thirdparty',  # implicit callable
    'thirdparty:init',  # explicit callable also OK
])

This is more intuitive (users just have to learn one thing), and we don't have to explain why and how things are different.

Update #2: reader.ua_fallback is not much harder to support, and still has (most) the benefits above.

@lemon24
Copy link
Owner Author

lemon24 commented Mar 24, 2021

To do:

  • move built-in plugins to reader.plugins.*, with the entry point renamed to init_reader
    • ua_fallback
    • enclosure_dedupe
    • feed_entry_dedupe
  • add make_reader(plugins=...) argument, with ('ua_fallback',) as default
  • make make_reader_from_config() pass plugins to make_reader() when applicable
  • tests
  • docs
    • API
    • guide
    • exceptions (API and guide)
    • list of built-in stable plugins
  • versionadded
  • changelog
  • (maybe) get rid of _plugins.Loader just refactored it
  • (maybe) vendor import_string from stdlib pkgutil.resolve_name

lemon24 added a commit that referenced this issue Mar 24, 2021
lemon24 added a commit that referenced this issue Mar 25, 2021
lemon24 added a commit that referenced this issue Mar 25, 2021
lemon24 added a commit that referenced this issue Mar 25, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 26, 2021
lemon24 added a commit that referenced this issue Mar 27, 2021
@lemon24 lemon24 closed this as completed Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant