-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Follow the Flask plugin model instead of namespace packages #3210
Conversation
@Nodd, thanks a lot for working on this one! Quick comments:
|
Or maybe |
@Nodd and @ccordoba12 I dont think adding the ui or io is worth it. IO things are just plugins... so making the difference by name is really just making the names ugly and long. We should just add some variable in the init of the plugin like from spyderlib.plugin import IO_PLUGIN, UI_PLUGIN
PLUGIN_TYPES = [IO_PLUGIN, UI_PLUGIN] # for a plugin that does the two things?
... And have short names like (pip/conda package and module name):
... |
@goanpeca From a certain point of view IO plugins are just plugins because they are additions to spyder, but they are actually very different from UI plugins:
Also the big problem with differentiating by module variables (apart from being a little magical) is that you have to load the plugin to check which kind it is. Considering how they are loaded and used very differently internally, I sill think it makes sense to differentiate them explicitly in their names. To be honest if we decide to refactor the way IO plugins are loaded, someone else will have to do it because I won't be able to spend time on this. To keep names shorter, I'd be in favor of having IO plugins starting with |
@Nodd, I understand how it works and how they load differently and how they might load differently. But IO plugins was just a special case of a general plugin that extends the variable explorer... so in a sense is a normal plugin that extends that functionality. It was not coded that way for whatever reason and it starts in a different part of the application for whatever reason, and trying to compensate for that seems like a bad idea. In general every normal plugin could/should be extendable.
I just dislike this very much and find very useful that modules and packages names relate to each other. I agree that this might be out of the scope of this PR, but I dont see any good reason to maintain the current way IO plugins load/work |
It's not "whatever reason". On one side a plugin is a function to load a file with a specific format and extension. On the other side a plugin has a complex API and explicitly needs a dockwidget (a menu-entry plugin needs a hack currently). Even if we name them both "plugins" they have a very different goal (I'm not even speaking about implementation). It's not just a naming problem, having 2 different interfaces for plugins is weird. I think that the most correct way would be to integrate IO plugins into UI plugins (maybe with a helper class to inherit from ?), feel free to break everything ! 😉
What do you think about this ? This way in the future we could change IO plugins only (merging them into ui plugins) while being backward compatible with ui plugins, and keeping the names clean. |
Yes, I would like to have a single plugin interface, that is the whole point :-)
Yes perfect. I would say the approach to take after this PR is:
|
WTF git ? I merged nothing ! |
?? |
All my commits were removed from the PR... |
@Nodd, it seems you changed all line endings of several files :-) |
😭 I edited some files on windows... I tought it was correctly configured, but it seems I was wrong. |
I put the EOL back to the mess they where. I still have to re-mess the whitespaces ! |
OK, everything should work now ! I also updated spyder.line_profiler and spyder.autopep8 (in my repo, master branch) if you want to test external plugins. |
Thanks a lot Joseph! We definitely need to move to normalize line endings!! Could prepare a new PR about that? I promise to merge it and adapt all El 08/06/16 a las 03:42, Joseph Martinot-Lagarde escribió:
|
line_profiler (the plugin) is still buggy, I ported memory_profiler but it's buggy too. You can test the vim plugin, I just did some work on it. |
I did some cleanups in the plugins, they should all work now. You have to use the versions from https://github.com/Nodd?tab=repositories, I'll push to spyder-ide when this PR is merged. |
... and I'm slowly starting to hate QuantifiedCode. I like nice-looking code, but this thing is so mechanical that the few useful warnings are lost in the noise. |
@Nodd, is this one ready? I think the only thing left is to fix |
I updated the gettext scripts and setup.py (without testing the changes...). Otherwise it should be ready, I don't know if someone tested on another configuration than my machine. |
loads its submodules.""" | ||
namespace_path = osp.join(plugin_path, base_namespace, plugins_namespace) | ||
PLUGIN_PREFIX = "spyder_" | ||
IO_PREFIX = PLUGIN_PREFIX + "io_" |
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.
Question here: What's the name for io
plugins according to this? Is it spyder_io_foo
or spyderio_foo
?
I say that because you named our io plugins like spyderio_foo
, so those names or this line should be changed :-)
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.
spyder_io_foo
. I forgot to rename the files.
'spyder_profiler': get_package_data('spyder_profiler', EXTLIST), | ||
'spyder_pylint': get_package_data('spyder_pylint', EXTLIST), | ||
'spyderio_dcm': get_package_data('spyderio_dcm', EXTLIST), | ||
'spyderio_hdf5': get_package_data('spyderio_hdf5', EXTLIST), |
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.
@Nodd, these lines also need to be changed to spyder_io_foo
:-)
Except for a minor detail in |
Stupid me, I modified |
The problem in Travis had to do with new conda packages for PyQt5. They had the wrong dependencies, so I removed them for now :-) I have to recompile Qt5 to fix that problem. |
I updated the master branch of all "my" plugins in spyder-ide so that they are compatible. Note that it's not the case for spyder.example ! (and spyder.terminado but this one is empty) |
After this merge I am not getting plugins from bdist_wheel. |
@TWAC You're right, thank you for noticing. I sent a PR to correct this. |
Here is a quick prototype for loading plugins using the flask model.
Fixes #3150.
I went with the two different prefixes because it was simpler, it can be changed in the future.
I still have to look precisely at how to load a module, because from what I saw more functions were deprecated in python 3.5.
TODO: