Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Namespace-based Export Plugins #1006

Closed
MinchinWeb opened this issue Jul 25, 2020 · 22 comments
Closed

Namespace-based Export Plugins #1006

MinchinWeb opened this issue Jul 25, 2020 · 22 comments
Labels
discussion Open-ended conversation about a topic

Comments

@MinchinWeb
Copy link
Contributor

I'm thinking about reworking the export plugin system to be auto-discovered based on the module name. Assuming this works well, it could be expanded to import plugins and possibly the storage backends as well.

There would be two namespaces: one for internal plugins, and another for external/contributed plugins.

The usecase would be to solve issues such as #779 where there is a difference of opinion of what the export should look like, or if you have a special format that you need. "Contributed" plugins could also be a way to test out a format before added in to the core of jrnl.

For myself, I have a "special" plugin that I used as a prep step to feed my jrnl entries into Pelican (a static site generator). It's always seemed like too "special" of a case to submit for the main repo, but it's something I use regularly. With the support and bug fixes to DayOne support, this is probably the only reason I have left to keep a private fork of jrnl.

In terms of implementation, I'm imagining that each plugin would have five methods:

  • one that returns it's name (e.g. "Markdown Exporter")
  • one that returns it's version (e.g. "2.4.4-beta"; for built-in plugins, this would match jrnl)
  • one that returns a list of "format codes" that it will process (e.g. ["md", "markdown"]; this is was will used by the export command of jrnl)
  • one that is passed a single entry and returns the exported entry
  • one that is passed a journal and returns the exported journal

I expect that contributed plugins would override the build-in plugins if they defined the same format code.

By using namespaced plugins, the plugin would be install-able via pip and would require no more configuration or activation beyond being importable by Python.

I haven't written any code yet (but I've been trying to figure out how to do this for probably 3 years...), but I wanted to get some thoughts on if there was support for this before I spend a bunch of time on it.

Thanks,

@MinchinWeb MinchinWeb added 🆕 New! enhancement New feature or request labels Jul 25, 2020
@wren wren added discussion Open-ended conversation about a topic and removed enhancement New feature or request labels Jul 25, 2020
@wren wren changed the title [Discussion] Namespace-based Export Plugins Namespace-based Export Plugins Jul 25, 2020
@wren
Copy link
Member

wren commented Jul 26, 2020

Thanks for bringing this up; I've also been wanting to get our plugins re-done.

I'm still thinking through this, but here are my thoughts so far.

  • Our current system doesn't mesh well with distribution methods. This is due to the fact that plugins are expected to be loaded from a directory the jrnl core. This works just fine if you're running jrnl from source, but anywhere else (pipx, brew, pip, etc) there's a decent chance your plugin will be uninstalled on upgrade. The best way to deal with this at the moment is to either reinstall your plugin (copy the file into the correct folder in jrnl) after each upgrade, or maintain a private fork (as you mentioned) where you basically have a patch for your plugin on top of the jrnl source. Both are kind pretty annoying, and users really shouldn't have to deal with either of them.

  • Significantly changing the way plugins work is a breaking change, and seems like a major change in increment semantic versioning terms (meaning we update the version to v3.0.0). In and of itself that's not a bad thing, and I'm very okay with doing it, but I do want to be sure we think through the changes we want, and that we're "getting our money's worth" for a major change.

  • The current plugins can't really do very much other than handle formatting, and aren't hugely different from piping the data to an external program that does the same thing. I would love for our plugin system to allow for better flow control, and even creating new journal types.

  • I don't think the jrnl core should support very many journal types, but having a plugin system that can handle that would allow the community to create as many different types as it needs, without needing to do any of the above jankyness to keep their plugins installed.

  • This kind of system would definitely be an increase in scope from what you are discussing in the OP. What would be the best way to split this work up in a way that benefits users more quickly so that some big jrnl v3 release isn't forever on the horizon?

  • Can we fix the first point up top (about reinstalling plugins on upgrade) be fixed without breaking changes to the current plugin system? Maybe something along the lines of a user-defined directory (maybe in config) to load plugins from in addition to the jrnl core plugins? This solve an immediate problem for users while buying us more time to implement the more in-depth changes to the plugins.

  • How do hooks (Application hooks #921) factor into this (if at all)? Could implementing hooks (maybe in conjunction with a user plugin directory?) help us solve some of those same more immediate problems?

That's all I've got for now. Sorry it's disorganized, but I'm still mulling over all of it.

@wren
Copy link
Member

wren commented Jul 26, 2020

Oh, I forgot to mention pluggy. It looks like a very useful, lightweight library for managing this type of problem. We'd just have to design a spec for what we wanted, and it can handle pretty much the rest of it.

@wren
Copy link
Member

wren commented Aug 6, 2020

I'm kinda liking the approach of moving the plugin directory (so a user can just drop in their plugins independent of the jrnl core plugins). This would buy us some time to redesign the plugin system while not breaking backwards compatibility (for now).

What do you think, @MinchinWeb?

@MinchinWeb
Copy link
Contributor Author

Hey @wren ! I think your vision is fairly close to mine, so I'll start working on something!

@micahellison micahellison removed the 🆕 New! label Aug 8, 2020
@stale
Copy link

stale bot commented Oct 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Inactive issue: will be closed soon if no activity label Oct 7, 2020
@MinchinWeb
Copy link
Contributor Author

still planning on making this happen!

@stale stale bot removed the stale Inactive issue: will be closed soon if no activity label Oct 8, 2020
@stale
Copy link

stale bot commented Dec 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Inactive issue: will be closed soon if no activity label Dec 8, 2020
@MinchinWeb
Copy link
Contributor Author

not stale...

@stale stale bot removed the stale Inactive issue: will be closed soon if no activity label Dec 8, 2020
@wren
Copy link
Member

wren commented Dec 13, 2020

@MinchinWeb How's this going? Any progress? Is there anything we can help with? I'd love to get the solution we talked about in to jrnl soon, since it'll make it easier to make updates to the exporters without worrying about breaking them for users.

@stale
Copy link

stale bot commented Feb 14, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Inactive issue: will be closed soon if no activity label Feb 14, 2021
@MinchinWeb
Copy link
Contributor Author

bump

@stale stale bot removed the stale Inactive issue: will be closed soon if no activity label Feb 17, 2021
@stale
Copy link

stale bot commented Jun 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Inactive issue: will be closed soon if no activity label Jun 3, 2021
@MinchinWeb
Copy link
Contributor Author

not stale, still WIP

@stale stale bot removed the stale Inactive issue: will be closed soon if no activity label Jun 3, 2021
@wren
Copy link
Member

wren commented Jun 5, 2021

Oh, ha. It looks like stalebot is working again. It had gone down for a while and I honestly wasn't sure if they were ever going to fix it.

@micahellison
Copy link
Member

This code is currently in the merged PR in the feature-plugins branch. There's a little work left to do before merging it in:

  • plugins listed in jrnl --version should be in jrnl --diagnostic
  • deal with folder-changing issue by removing the two init.py files from jrnl and moving their functionality elsewhere (I think @wren has this stashed somewhere)
  • documentation changes
  • simplify the CI/testing workflows - ideally it should use poetry, and the makefile should support it

There's one more thing that I want to change, though it's not a showstopper: ideally, the feature file should be able to use a "Given" step to install and test plugins. When we start getting issues from plugin developers, this will make it a lot easier to prevent regressions.

@micahellison
Copy link
Member

Working with this today, I noticed that there aren't any tests for import plugins. I think we need to add those as well.

@micahellison
Copy link
Member

It looks like the sample importer plugin isn't writing to the journal at all - it reports that it is, but the journal doesn't show any data added to it. It looks like a write command needs to be sent, but that seems a little redundant for a plugin -- maybe it should automagically write instead of requiring the plugin author to tell it to write.

There's also some funny stuff going on with the plugin's read logic, grabbing data differently based on whether it's coming from stdin vs. a file, and not appropriately reading from the file if it is a file.

@MinchinWeb
Copy link
Contributor Author

Further to #1275:

  • I think we want to keep the __main__.py file. That allows you to run jrnl as a module; i.e. python -m jrnl. I can't find the issue or pull request related to the implementation at the moment. I think it would be good to maintain this functionality; I don't see much downside in doing so. Restore __main__.py functionality #1278 restores this.
  • I'm not crazy about the double in the import path jrnl.__version__.__version__, I wish this could be available at jrnl.__version__.
  • The version imports for the plugins was purposefully a realitive import. Use relative version imports for plugins #1280 restores this.

ideally, the feature file should be able to use a "Given" step to install and test plugins.

Now that we have the __init__.py files sorted, could this be as simple as adding the plugin filepath to the Python's Path?


It looks like the sample importer plugin isn't writing to the journal at all - it reports that it is, but the journal doesn't show any data added to it. It looks like a write command needs to be sent, but that seems a little redundant for a plugin -- maybe it should automagically write instead of requiring the plugin author to tell it to write.

There's also some funny stuff going on with the plugin's read logic, grabbing data differently based on whether it's coming from stdin vs. a file, and not appropriately reading from the file if it is a file.

#1281 deals with this:

  • explicitly write the journal to disk (after importing entries), rather than assuming the plugin will do this
  • be ore consistent in how data fed to the custom JSON importer is dealt with (i.e. pharse both input files and stdin into JSON)
  • first pass at a test for the custom JSON importer. The test isn't running for me locally, but also isn't displaying any error message, so I'm not sure what's going on

@wren
Copy link
Member

wren commented Jul 5, 2021

I think we want to keep the __main__.py file. That allows you to run jrnl as a module; i.e. python -m jrnl. I can't find the issue or pull request related to the implementation at the moment. I think it would be good to maintain this functionality; I don't see much downside in doing so. Restore __main__.py functionality #1278 restores this.

Yup! Agreed and merged.

I'm not crazy about the double in the import path jrnl.__version__.__version__, I wish this could be available at jrnl.__version__.

Yeah, I'm not jazzed about it either. Not sure if that's doable without the init files, though. Maybe we can move to something better like jrnl.constants or something?

The version imports for the plugins was purposefully a realitive import. Use relative version imports for plugins #1280 restores this.

I commented on the PR about not understanding the use case, but I wanted to bring up a separate point here. If the version for each plugin is always the same, and it's always imported from jrnl, then why do we even store or display it? I think we should either:

  1. Get rid of version numbers in plugins, or
  2. Have each plugin store its own meaningful version number.

I think #1 makes sense for core plugins (the ones that ship with jrnl), and #2 makes sense for third-party plugins (contrib). What do you think?

ideally, the feature file should be able to use a "Given" step to install and test plugins.

Now that we have the __init__.py files sorted, could this be as simple as adding the plugin filepath to the Python's Path?

Yup, I think so (at least, that's the same thing I was thinking). We should be able to conditionally copy the plugin filepath to the temp folder that the tests run in (pyest-bdd, not behave).

@MinchinWeb
Copy link
Contributor Author

I'm not crazy about the double in the import path jrnl.__version__.__version__, I wish this could be available at jrnl.__version__.

Yeah, I'm not jazzed about it either. Not sure if that's doable without the init files, though. Maybe we can move to something better like jrnl.constants or something?

I can make jrnl.__version__ work, the code is simple; I'm just not sure where to put the code:

import jrnl

setattr(jrnl, "__version__", "2.8.0")

The version imports for the plugins was purposefully a realitive import. Use relative version imports for plugins #1280 restores this.

I commented on the PR about not understanding the use case, but I wanted to bring up a separate point here. If the version for each plugin is always the same, and it's always imported from jrnl, then why do we even store or display it? I think we should either:

1. Get rid of version numbers in plugins, or
2. Have each plugin store its own meaningful version number.

I think #1 makes sense for core plugins (the ones that ship with jrnl), and #2 makes sense for third-party plugins (contrib). What do you think?

I've been thinking about this, and trying to come up with a plausible scenario where it might make a difference, and mostly coming up empty.

One place where it might make a difference is when the plugin code is used as demo code for others writing their own plugins; I don't want them to set their version number to simply be jrnl's.

As for 1, I think it's important/helpful to have a version number for debuging things when they break, at least for external plugins. By simply giving the built in plugins a version number, we don't have to deal with the special case of some plugins having version numbers and others not. By keeping internal and external plugins as close in implementation as possible, it makes it increadibly simple to move plugins from internal to external (which I think will prove helpful down the road).

As for 2 (giving each internal plugin its own meaningful version), I'm not opposed to this as such, just the extra work involved is something to be aware of. As (internal) plugins will ship only with a specific version of jrnl, using jnrl's version number seemed like a way to provide a version number without any extra work.

@wren
Copy link
Member

wren commented Jul 8, 2021

I can make jrnl.__version__ work, the code is simple; I'm just not sure where to put the code:

import jrnl

setattr(jrnl, "__version__", "2.8.0")

TIL about setattr. Thank you!

But--and maybe I'm reading your suggestion wrong--wouldn't putting import jrnl anywhere inside the jrnl module itself cause a circular dependency?

1. Get rid of version numbers in plugins, or
2. Have each plugin store its own meaningful version number.

I think #1 makes sense for core plugins (the ones that ship with jrnl), and #2 makes sense for third-party plugins (contrib). What do you think?

I've been thinking about this, and trying to come up with a plausible scenario where it might make a difference, and mostly coming up empty.

One place where it might make a difference is when the plugin code is used as demo code for others writing their own plugins; I don't want them to set their version number to simply be jrnl's.

I agree we don't want third-party plugins hooking into the jrnl version. In that case, wouldn't we want to avoid showing them how to do it in the first place?

I think #1 makes sense for core plugins (the ones that ship with jrnl),

As for 1, I think it's important/helpful to have a version number for debuging things when they break, at least for external plugins. By simply giving the built in plugins a version number, we don't have to deal with the special case of some plugins having version numbers and others not. By keeping internal and external plugins as close in implementation as possible, it makes it increadibly simple to move plugins from internal to external (which I think will prove helpful down the road).

I personally don't think not having a version is a big deal, but I see what you're saying about keeping the implementations as close as possible. We could also leave the version as is and just not do anything with it (like in the diagnostic page we could just leave version out for core plugins).

#2 makes sense for third-party plugins (contrib)

As for 2 (giving each internal plugin its own meaningful version), I'm not opposed to this as such, just the extra work involved is something to be aware of. As (internal) plugins will ship only with a specific version of jrnl, using jnrl's version number seemed like a way to provide a version number without any extra work.

To clarify, I was suggesting #2 for third-party plugins. So, it sounds to me like we're on the same page for this.

@MinchinWeb
Copy link
Contributor Author

But -- and maybe I'm reading your suggestion wrong -- wouldn't putting import jrnl anywhere inside the jrnl module itself cause a circular dependency?

Surprisingly no. Maybe this is because it's an explicit import? The one place you can't put it is in the __version__.py file itself, as somewhere after that file is imported, the reference is still to the module. So taking it a step further, #1296 just overrides the module!

I agree we don't want third-party plugins hooking into the jrnl version. In that case, wouldn't we want to avoid showing them how to do it in the first place?

I suppose my logic was that by showing them that a version is defined, they'll know they need to provide one. But a guess someone new could draw either conclusion.

We could also leave the version as is and just not do anything with it (like in the diagnostic page we could just leave version out for core plugins).

Do we also leave out the import paths?

@jrnl-org jrnl-org locked and limited conversation to collaborators Jul 31, 2021
@wren wren closed this as completed Jul 31, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
discussion Open-ended conversation about a topic
Projects
None yet
Development

No branches or pull requests

3 participants