Improve plugin loading error messages and logging for plugin developers #2686
Conversation
|
Improve plugin loading error messages and logging for plugin developers |
| if not hasattr(plugin_module, attr): | ||
| log.error(f"[pluginmanager] Plugin {name} missing required attribute: {attr}") | ||
| else: | ||
| log.info(f"[pluginmanager] Plugin {name} has attribute: {attr} = {getattr(plugin_module, attr)}") |
There was a problem hiding this comment.
IMO should be log.debug not log.info .
| del sys.modules[full_module_name] | ||
| raise | ||
|
|
||
| # --- BEGIN: Verbesserte Logging-Ausgaben für Plugin-Entwickler --- |
There was a problem hiding this comment.
IMO this comment is unnecessary - this error should be reported anyway.
But if there is going to be a comment then it needs to be in the project locale of English.
| required_attrs = ["PLUGIN_NAME", "PLUGIN_API_VERSIONS"] | ||
| for attr in required_attrs: | ||
| if not hasattr(plugin_module, attr): | ||
| log.error(f"[pluginmanager] Plugin {name} missing required attribute: {attr}") |
There was a problem hiding this comment.
-
This should probably be using
plugin_errorto log an error. -
If it doesn't have the required attributes, should it immediately return rather than continuing to try to load?
| raise | ||
|
|
||
| # --- BEGIN: Verbesserte Logging-Ausgaben für Plugin-Entwickler --- | ||
| required_attrs = ["PLUGIN_NAME", "PLUGIN_API_VERSIONS"] |
There was a problem hiding this comment.
This needs to be a global constant rather than being initialised individually as each plugin is attempted to be loaded.
| else: | ||
| self.plugins.append(plugin) | ||
| else: | ||
| log.error(f"[pluginmanager] Plugin {plugin.name} is not compatible with API version(s) {getattr(plugin_module, 'PLUGIN_API_VERSIONS', None)}. Picard supports: {picard.api_versions_tuple}") |
There was a problem hiding this comment.
The plugin_error function already logs an error. This line should be removed, and if a better error message is needed then a change to the following line should be made.
…ogging und Plugin-Kompatibilität verbessert
- New QAction 'Watch Folders' with toggle in toolbar - Default toolbar layout updated - Added config options watch_folders_auto_tag and watch_folders_auto_save - UI enums and actions updated - WatchFolderManager supports start/stop and auto-tag/save placeholders - MainWindow method to control monitoring via toggle
| logging.basicConfig(filename='/home/tobber/picard_file_hook_debug.log', level=logging.DEBUG) | ||
| logging.debug(f"run_file_post_load_processors aufgerufen für: {file_object}") | ||
| logging.debug(f"Registrierte file_post_load_processors: {file_post_load_processors.functions}") | ||
| file_post_load_processors.run(file_object) |
There was a problem hiding this comment.
These logging lines are development and need to be removed.
| return (None, None) | ||
|
|
||
| def _load_plugin(self, name): | ||
| def _load_plugin2(self, name): |
| else: | ||
| log.warning(f"[pluginmanager] Plugin {name} stellt keine bekannten Hooks für Picard 3.x bereit. Das Plugin wird wahrscheinlich nicht ausgeführt.") | ||
| # --- END: Hook-Übersicht und Warnungen --- | ||
|
|
There was a problem hiding this comment.
I have ZERO idea what the functionality in this latest commit is supposed to do or why it is needed. But it now seems to mix at least two and possibly three separate pieces of functionality that should be submitted in separate PRs.
As per previous review, found hooks logging should be log.debug, and comments and log messages should be in English and with translation enabled if you wish.
Leaving aside the previous warning about this not being merged due to a revamp of the entire plugins approach, as it stands there are (IMO as someone who doesn't have merge permissions) several reasons why this code is not up the quality standards needed for it to be merged.
| log.info(f"[pluginmanager] Workaround: {hook_func} im Plugin {name} explizit aufgerufen.") | ||
| except Exception as e: | ||
| log.error(f"[pluginmanager] Fehler beim expliziten Aufruf von {hook_func} in Plugin {name}: {e}") | ||
| # --- END: Workaround --- |
There was a problem hiding this comment.
More comments and log messages in German, more log.info messages that should be log.debug, and more lists that should be global or class constants. And it doesn't seem like a workaround either - but I am not sure what it is supposed to do or indeed whether hasattr actually does what you think it is going to do.
| """Initialize and load plugins""" | ||
| import logging | ||
| logging.basicConfig(filename='/home/tobber/picard_startup_debug.log', level=logging.DEBUG) | ||
| logging.debug("_init_plugins wird ausgeführt") |
There was a problem hiding this comment.
More development code that shouldn't be in a PR.
| """Initialize tagger objects/entities""" | ||
| import logging | ||
| logging.basicConfig(filename='/home/tobber/picard_startup_debug.log', level=logging.DEBUG) | ||
| logging.debug("_init_tagger_entities wird ausgeführt") |
There was a problem hiding this comment.
Yet more development code that shouldn't be in a PR.
| """Add files to the tagger.""" | ||
| import logging | ||
| logging.basicConfig(filename='/home/tobber/picard_startup_debug.log', level=logging.DEBUG) | ||
| logging.debug(f"add_files wird aufgerufen mit: {filenames}") |
There was a problem hiding this comment.
Even more development code that shouldn't be in a PR.
| log.info("WatchFolder: gestartet") | ||
| else: | ||
| self.tagger.watchfolders.stop() | ||
| log.info("WatchFolder: gestoppt") |
There was a problem hiding this comment.
More log.info that should be log.debug.
|
In addition, this PR is marked as a BUG FIX, but IMO it is not addressing any identified bugs but instead introducing several new pieces of functionality, |
…ools, Watchfolder, Tag-Vorschläge, UI- und Fehleroptimierungen
There was a problem hiding this comment.
I'm sorry, but this PR in it's current state has too many issues.
- The PR claims to improve the logging. But actually the logging added is mostly purely local debug code, which writes some messages to some hard coded file.
- The plugin loading code was extended to call some functions in the plugin. This seems to be the result of a general misunderstanding of how the plugins and the hooks work.
- The PR contains a partial implementation of some "watch folder" functionality. This should be a separate PR, with full functionality. Overall this seems like an idea that should be further investigated, but it would need some further discussion, as I don't think all the ideas shown by the new options would make sense (e.g. I would be strictly against the auto fingerprinting lookup + save function).
- The theme menu should also be a separate PR, and likely also needs some more consideration
- All comments and doc strings need to be in English
As I wrote on the forums the plugin code in the master branch is subject to change. In theory plugin loading and execution of Picard 2 style plugins should still work, but is untested. The plugin UI is currently non functional (I have removed it in my development branch, we'll need to redo it anyway). Please see #2419 and and my branch https://github.com/phw/picard/tree/plugins-v3-cli if you are really interested. But things are still all still changing and I wouldn't recommend to base any work on those branches right now.
What I would actually suggest is that you first try to implement the plugin you want to write so it works with current Picard 2. Please have a look at https://picard-docs.musicbrainz.org/en/tutorials/write_plugin.html and the existing plugins in https://github.com/metabrainz/picard-plugins/tree/2.0/plugins .
Please feel free to post your plugin, even if it is only partial and not functional, on the forums and I will be happy to help you implement this. Also in German, if you prefer.
| ListOption('setting', 'watch_folders_paths', [], title=N_("Watch folders paths")) | ||
| BoolOption('setting', 'watch_folders_autostart', True, title=N_("Start watch folders on startup")) | ||
| BoolOption('setting', 'watch_folders_auto_tag', True, title=N_("Automatically tag new files in watch folders")) | ||
| BoolOption('setting', 'watch_folders_auto_save', False, title=N_("Automatically save after tagging files from watch folders")) |
There was a problem hiding this comment.
These options are all unrelated to the pull request and should not be included. This seems to be about a new watchfolder functionality, that is only partially implemented (not all the above options are actually used). In any case this should be part of a separate PR.
| self.plugins.append(plugin) | ||
| # --- BEGIN: Workaround: Hook-Registrierungsfunktionen explizit aufrufen --- | ||
| # Versuche, alle bekannten Hook-Registrierungsfunktionen aufzurufen | ||
| tagger_instance = None |
There was a problem hiding this comment.
I think this code and the "workaround" it implements are actually caused by a misunderstanding of how the plugin hooks actually work. The plugins are not supposed to contain a function called e.g. register_file_post_load_processor. Instead they are supposed to import this function from Picard and call it to register the thing. Please have a look at https://picard-docs.musicbrainz.org/en/tutorials/write_plugin.html and the existing plugins for examples.
But as it is the code here should not be added and is actually implementing unexpected behavior.
|
|
||
| def _load_plugin(self, name): | ||
| def _load_plugin2(self, name): | ||
| global picard |
| from picard.util import is_hidden | ||
|
|
||
| # Sehr grobe Auswahl gängiger Audio-Extensions | ||
| AUDIO_EXTS: Set[str] = { |
There was a problem hiding this comment.
No need to define this here. All known extensions could already be retrieved with picard.formats.util.supported_extensions.
Actually it might not be necessary, as Tagger.add_files would already only load valid files. But maybe for the watch feature it would be useful to reduce the number of attempted loads for performance issues.
|
Tut mir leid, aber ich werde diesen PR jetzt schließen. Es gab weiter oben bereits mehrere Kommentare zu den Problemen mit diesem PR und warum er in seiner aktuellen Form nicht zielführend ist. Statt jedoch auf diese Kommentare einzugehen oder zumindest das Geschriebene zu berücksichtigen, hast du diesen PR um noch mehr themenfremde Funktionalitäten erweitert (Plugin-"Health"-Reporting, Performance-Monitoring, willkürliche Änderungen an Logmeldungen, Dialog für Tag-Vorschläge, weitere hartkodierte deutsche Strings und wahrscheinlich noch mehr). Ein Großteil des Codes scheint zudem nicht vollständig funktionsfähig zu sein. Jedes dieser Themen sollte ein eigener PR sein. Wenn du eines dieser Themen ernsthaft weiterverfolgen möchtest, konzentriere dich bitte auf ein einzelnes Problem und öffne einen separaten PR nur für dieses eine Feature oder den einen Fix. Beim Öffnen eines PRs sollte sich der Code in einem vernünftigen Zustand befinden, in dem die Kernfunktionalität funktioniert und der Code zur Diskussion bereit ist. Bitte öffne derzeit nicht mehr als einen PR. Ich habe aktuell kein Interesse daran, mehr als einen PR dieser Art zu reviewen. Wenn du dich auf ein einzelnes Thema mit einer klaren Beschreibung der Ziele, könnten wir damit eventuell weiterkommen. Ich muss betonen, dass deine aktive Teilnahme an der Diskussion notwendig sein wird. Du hast zwar eine Diskussion im Forum und diesen PR gestartet, aber nie auf das gegebene Feedback reagiert. -- Sorry, I'm going to close this PR now. There have been several comments above about the issues with this PR and why in its current form it will not lead anywhere. But instead of responding to those comments or at least considering what has been written you extended this PR with yet more unrelated functionality (plugin "health" report, performance monitoring, random changes to log messages, tag suggestion dialog, more German hard coded strings and probably more). Much of the code does not seem to be fully functional. Any of those different topics should be their own PR. If you seriously want to bring one of those topics forward please focus on one issue and open a single PR for only a single feature or fix. When opening a PR the code should be in a reasonable state where the core functionality is working and the code is ready for discussion. Please do not open more than one PR at this time. At this point I really have no interest in reviewing more than one PR similar to this one. If you focus on a single topic with a clear description on the goals we could probably move this forward. I have to emphasize that your active participation in the discussion will be necessary. You have opened a discussion on the forums and this PR, but never responded to any feedback given. |
There was a problem hiding this comment.
I have stopped reviewing this latest commit part way through because @bumblei3 is not willing to learn from previous review comments and further effort is pointless.
There are potentially a LOT of good ideas in here that I would love to see proposed properly - but unfortunately it is a pity that they are IMO unlikely ever to get included in Picard unless @bumblei3 is prepared to learn the following:
- English is the project language and comments and error messages need to be in English (with gettext translation as appropriate).
- Picard has evolved to be programmed in a certain way so that it is reliable - most notably how threads are used and how the UI needs to be called so that it is guaranteed to be on the main thread - and new code needs to follow these approaches.
- Picard also has a coding style and a change management approach (discussion, tickets, PRs) - and new code needs to follow these too.
All of the above are not a problem if the user is prepared to learn from review comments - but a user who insists on using his own style / code against the advice of reviewers will likely never get a PR merged.
| from picard.tag_suggestions import generate_suggestions # Lazy import, vermeidet Zirkeln | ||
| suggestions = generate_suggestions(self._new_metadata, self._release_node) | ||
| # Speichere Vorschläge im Album-Objekt für spätere UI-Nutzung | ||
| self.tag_suggestions = suggestions # type: ignore[attr-defined] |
There was a problem hiding this comment.
This new code has nothing to do with plugin loading error messages.
| user_msg = ("Das Laden der Album-Informationen ist fehlgeschlagen. " | ||
| "Bitte prüfen Sie Ihre Internetverbindung oder versuchen Sie es später erneut.\n\n" | ||
| f"[Technischer Fehler: {err_str}]") | ||
| self.error_append(user_msg) |
There was a problem hiding this comment.
In principle I am supportive of an improvement to this error message - it would be beneficial to add a description saying that it is an http error, as well as the contents of the actual error message, but as previously stated error messages need to be in English, though they can be translated - and in any case this isn't the correct way to insert parameters into strings.
| from PyQt6 import QtWidgets | ||
| from picard.i18n import gettext as _ | ||
| config = get_config() | ||
| if not config.setting['show_tag_suggestions']: |
There was a problem hiding this comment.
More new functionality not related to plugin loading error messages.
| def _show_tag_suggestions(self): | ||
| """Zeigt einen Dialog mit Tag-Vorschlägen und wendet sie ggf. an.""" | ||
| from PyQt6 import QtWidgets | ||
| from picard.i18n import gettext as _ |
There was a problem hiding this comment.
Imports should probably be at the file level not at the function level.
|
|
||
| def _show_tag_suggestions(self): | ||
| """Zeigt einen Dialog mit Tag-Vorschlägen und wendet sie ggf. an.""" | ||
| from PyQt6 import QtWidgets |
There was a problem hiding this comment.
Not sure that UI activities should be in this module.
| ] | ||
|
|
||
| for tip in tips: | ||
| log.info(f"[plugin-dev] {tip}") |
There was a problem hiding this comment.
Yet another whole new tranche of functionality that needs to be in a separate PR.
|
|
||
|
|
||
| # Global instance | ||
| plugin_health_monitor = PluginHealthMonitor() |
There was a problem hiding this comment.
Yet another whole new tranche of functionality that needs to be in a separate PR.
| with the `params` keyword parameter. This is specifically useful to | ||
| pass a dictionary when using named placeholders.""" | ||
| Statt einer rein technischen Meldung wird eine deutschsprachige, benutzerfreundliche Meldung erzeugt. | ||
| """ |
There was a problem hiding this comment.
FFS - it is bad enough when you add new comments in German, but translating existing comments from English to German is even worse.
| log_func(error) | ||
| self.plugin_errored.emit(name, error, False) | ||
| # Benutzerfreundliche deutsche Meldung | ||
| user_msg = f'Das Plugin "{name}" konnte nicht geladen werden. Bitte prüfen Sie, ob das Plugin mit dieser Picard-Version kompatibel ist oder kontaktieren Sie den Plugin-Autor.\n\n[Technischer Fehler: {error}]' |
There was a problem hiding this comment.
Yet another error message in German.
| if language: | ||
| suggestions["language"] = language | ||
|
|
||
| return suggestions No newline at end of file |
There was a problem hiding this comment.
Line end missing.
And yet another tranche of unrelated functionality. And more German comments.
|
sry... i'm trying vs code for the first time... |
|
There can be a steep learning curve to both programming in general and programming for a specific application like Picard. So here's my advice as someone who (in 2013) had his first 3 PRs not merged and indeed only had 3 of his first 10 PR attempts merged (and one of those was trivial)...
And a few don'ts:
|
|
do you guys have a discord server?? |
|
Matrix. |
@bumblei3 there are Matrix rooms, but you can also connect with Discord: https://musicbrainz.org/doc/Communication/ChatBrainz There is a musicbrainz-picard-dev room on Matrix. Not sure what the Discord name is, though. |
|
The Discord server is:
But aside from that its perfect. |
That used to be that way, but actually it is bridged now. There are quite some Discord users around |
|
@phw Phillip If Discord is integrated may I suggest:
|
|
@Sophist-UK you can suggest that, but not to me. I'm not using Discord and don't know how the channels are named there. But please have a look at https://musicbrainz.org/doc/Communication/ChatBrainz , IRC and Discord are both listed as ways to join the chat there.
There is already a Picard development channel (https://matrix.to/#/#musicbrainz-picard-dev:chatbrainz.org) and I see Discord users in there. I don't think a separate support channel makes sense, the MB community channel works well for that. A support channel only makes sense if someone is actually answering requests, and the community channel has enough participants that might be able to answer. The devel channel for Picard also only exists so zas and I don't need to have all discussions in private, because we frequently collided with other discussions and meetings on the metabrainz channel |
4 participants
Summary
Problem
Solution
Action
Additional actions required: