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

generate errors from bad <logfile> sections earlier #55

Merged
merged 3 commits into from
Dec 13, 2018

Conversation

freddrake
Copy link
Contributor

  • allow disallowed combinations of options be be identified
    and reported earlier (before any logfile handlers are created, and
    before any files are opened)

  • ensure DataConversionError is not converted to
    ConfigurationSyntaxError when a section end is loaded

- allow disallowed combinations of options be be identified
  and reported earlier (before any logfile handlers are created, and
  before any files are opened)

- ensure DataConversionError is not converted to
  ConfigurationSyntaxError when a section end is loaded
@freddrake
Copy link
Contributor Author

Risks: Changes can affect 3rd-party handlers that derive from ZConfig.components.logger.handlers.FileHandlerFactory.

I'd expect the real risk of this is low, but can be difficult to predict, and it could take some time for affected users to report problems.

Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

LGTM, but this might need a big version bump and a changelog notice about the earlier point of binding of sys.stdout/sys.stderr.

Copy link
Member

@jamadden jamadden left a comment

Choose a reason for hiding this comment

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

LGTM. I like the earlier error reporting, and I like not changing exception types unnecessarily (though I don't think I saw any tests that specifically check the type of exception?).

I agree that this needs a prominent change note and is probably technically incompatible enough that it deserves a major bump.

@freddrake
Copy link
Contributor Author

freddrake commented Dec 13, 2018

Agree with you both on needing a change note; realized I'd not done that while in the shower.

The earlier change, with delay and encoding support, included an update to 3.4.0, so I think the feature-added bump is fine. Were you thinking this should be a backward-incompatibility bump (to 4.0.0) instead?

@mgedmin
Copy link
Member

mgedmin commented Dec 13, 2018

I don't know. To me 3.3.x -> 3.4.0 is not scary, nobody's going to carefully look the changelog for such an update. But then the chances of this being a breaking change also seem small, so maybe that's fine?

@freddrake
Copy link
Contributor Author

On thinking further, the change in capture time of the STDOUT and STDERR streams is a detail tied to the implementation changes. I'm going to treat that as unintentional, and adjust so the capture time remains at handler construction.

capturing stderr, stdout earlier is not a specific goal; though unlikely
to be a problem for most uses, we can avoid the early capture to avoid
changing runtime behavior in a ways that can be surprising in way that
is unlikely to be detected early by consumers
@freddrake
Copy link
Contributor Author

Original capture time for stderr, stdout restored in commit f3fe237.

Added note to change history.

Still need tests that exceptions aren't changed inappropriately.

- check for more specific exceptions in tests
- check boundary values for logging levels
- extend early checking for option combinations to <email-notifier>
@freddrake
Copy link
Contributor Author

Done with changes; again ready for review.

@freddrake freddrake merged commit 2fa23e4 into master Dec 13, 2018
@freddrake freddrake deleted the fd-logfile-early-errors branch December 13, 2018 17:42
jugmac00 added a commit to zopefoundation/ZServer that referenced this pull request Apr 4, 2019
ZConfig 3.4.0 introduced some breaking changes with error handling.

cf zopefoundation/ZConfig#55

modified:   src/ZServer/Zope2/Startup/tests/test_warnfilter.py
jugmac00 added a commit to zopefoundation/ZServer that referenced this pull request Apr 9, 2019
ZConfig 3.4.0 introduced some breaking changes with error handling.

cf zopefoundation/ZConfig#55

modified:   src/ZServer/Zope2/Startup/tests/test_warnfilter.py
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.

None yet

3 participants