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

Add logging for TypeErrors in plugin settings #4241

Merged
merged 2 commits into from Oct 8, 2021

Conversation

ademuri
Copy link
Contributor

@ademuri ademuri commented Oct 1, 2021

  • Your changes are not possible to do through a plugin and relevant
    to a large audience (ideally all users of OctoPrint)
  • If your changes are large or otherwise disruptive: You have
    made sure your changes don't interfere with current development by
    talking it through with the maintainers, e.g. through a
    Brainstorming ticket
  • Your PR targets OctoPrint's devel branch if it's a completely
    new feature, or maintenance if it's a bug fix or improvement of
    existing functionality for the current stable version (no PRs
    against master or anything else please)
  • Your PR was opened from a custom branch on your repository
    (no PRs from your version of master, maintenance or devel please),
    e.g. dev/my_new_feature or fix/my_bugfix
  • Your PR only contains relevant changes: no unrelated files,
    no dead code, ideally only one commit - rebase and squash your PR
    if necessary!
  • Your changes follow the existing coding style
  • If your changes include style sheets: You have modified the
    .less source files, not the .css files (those are generated with
    lessc)
  • You have tested your changes (please state how!) - ideally you
    have added unit tests
  • You have run the existing unit tests against your changes and
    nothing broke
  • You have added yourself to the AUTHORS.md file :)

What does this PR do and why is it necessary?

This PR logs TypeErrors which occur inside of on_settings_save for plugins. I'm working on developing a plugin, and had a bug in my plugin which caused a TypeError in my override of this function. The log message incorrectly stated that I was calling octoprint.plugin.SettingsPlugin.on_settings_save incorrectly. This also changes that error message to indicate that calling that function incorrectly may be the cause.

How was it tested? How can it be tested by the reviewer?

I ran this with a plugin which generates a TypeError in the on_settings_save function. I also ran the unit tests.

What are the relevant tickets if any?

You can see that someone else was misled by this issue here: https://community.octoprint.org/t/solved-i-need-some-help-with-my-plugin/15732/13

@github-actions github-actions bot added the targets master The PR targets the master branch label Oct 1, 2021
@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Automatic PR Validation failed

There were one or more problems detected with this PR:

  • The PR's target branch master is not among the allowed target branches: maintenance, devel, staging/maintenance, staging/devel. Please only create PRs against these.

Please take a look at the Contribution Guidelines of this repository and make sure that the PR follows them. Thank you!

I'm just a bot 🤖 that does automatic checks, a human will intervene if I've made a mistake.

@github-actions github-actions bot added the needs some work There are some things to do before this PR can be merged label Oct 1, 2021
@ademuri ademuri changed the base branch from master to maintenance October 1, 2021 04:03
@GitIssueBot
Copy link

This pull request has been mentioned on OctoPrint Community Forum. There might be relevant details there:

https://community.octoprint.org/t/solved-i-need-some-help-with-my-plugin/15732/14

@cp2004 cp2004 removed the needs some work There are some things to do before this PR can be merged label Oct 1, 2021
Copy link
Member

@cp2004 cp2004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept meaning to sort this one out, but forgot several times... Thank you, should result in less confused plugin authors 👍

@foosel foosel added targets maintenance The PR targets the maintenance branch and removed targets master The PR targets the master branch labels Oct 8, 2021
@foosel foosel added this to the 1.8.0 milestone Oct 8, 2021
@foosel foosel merged commit 422e785 into OctoPrint:maintenance Oct 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
hacktoberfest-accepted targets maintenance The PR targets the maintenance branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants