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

Improper warnings on broken transform #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ewohnlich
Copy link

The broken.py module is particularly noisy. For reasons, I wanted to ignore some warnings from this module but encountered two issues when trying to set the threshold for the PortalTransforms logger above WARN.

  1. It doesn't just log the error, it also prints to the console
  2. The severity of the log message does not use logging.WARNING (30) it uses a global variable named WARNING that is set to 100. That is well above the level for CRITICAL (50)!!!

This pull request removes the print message and sets the severity to logging.WARNING.

@gforcada
Copy link
Member

@ewohnlich could you clarify the CLA status? The changes LGTM, once the CLA is clear we can merge right away 😃

@gforcada
Copy link
Member

Oh and the changelog entry! 😃

@ewohnlich
Copy link
Author

Ok, I have not contributed to Plone core before so I will need to sign the license agreement. For the changelog, should I just add a line in the bug fixes section in the 3.0.0 version?

@gforcada
Copy link
Member

Yes, thatw would be enough, thanks!

@mister-roboto
Copy link

Wohnlich, Eric (IMS) your emails are not known to GitHub and thus it is impossible to know if you have signed the Plone Contributor Agreement, which is required to merge this pull request.

Learn about the Plone Contributor Agreement: http://docs.plone.org/develop/coredev/docs/contributors_agreement_explained.html How to add more emails to your GitHub account: https://help.github.com/articles/adding-an-email-address-to-your-github-account/

@mister-roboto
Copy link

Wohnlich, Eric (IMS) your emails are not known to GitHub and thus it is impossible to know if you have signed the Plone Contributor Agreement, which is required to merge this pull request.

Learn about the Plone Contributor Agreement: http://docs.plone.org/develop/coredev/docs/contributors_agreement_explained.html How to add more emails to your GitHub account: https://help.github.com/articles/adding-an-email-address-to-your-github-account/

@ewohnlich
Copy link
Author

ewohnlich commented Dec 22, 2021

@mister-roboto I've set this account to display email address. I have signed the Plone contribution agreement and contributed more but I am not sure if it was with the personal gmail account set here or my account with my employer [email protected]. Please let me know if I need to update something.

edit: I didn't realize this was from such an old issue. I have no idea if it's still relevant.

@jensens
Copy link
Member

jensens commented Dec 23, 2021

@ewohnlich in git log you should see the email used for the commit.

@jensens
Copy link
Member

jensens commented Dec 23, 2021

btw., it is still an valid micro fix. Yesterday I looked through our old PRs to close a whole bunch, but yours is worth to get merged.

@Rudd-O Rudd-O self-requested a review December 25, 2021 13:18
Copy link

@Rudd-O Rudd-O left a comment

Choose a reason for hiding this comment

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

With CLA signed, this looks good to me.

@jensens
Copy link
Member

jensens commented Dec 25, 2021

In fact I would like to see a git amend with an email address connected to the email account. see https://www.git-tower.com/learn/git/faq/change-author-name-email or https://stackoverflow.com/questions/3042437/how-to-change-the-commit-author-for-one-specific-commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants