Skip to content

Allow installation of current 'structlog' major version 21.x #75

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

Merged

Conversation

grelleum
Copy link
Contributor

This PR allows using structlog major version 21 to be installed with diffsync.
This fixes issue: #74

Three changes are made:

  1. Pinned the pylint version to 2.6.x so that invoke tests will complete successfully after running poetry lock.
  2. Modified structlog versioning to allow 20.x.x or 21.x.x. Ran complete tests to verify compatibility.
  3. Updated poetry.lock file to include current structlog version.

@dgarros
Copy link
Contributor

dgarros commented Oct 28, 2021

Looks like there is a breaking change that we need to account for
https://www.structlog.org/en/stable/changelog.html#backward-incompatible-changes
Screen Shot 2021-10-28 at 12 53 46 PM
I did a quick search and we are using format_exc_info in

structlog.processors.format_exc_info,

@grelleum
Copy link
Contributor Author

grelleum commented Nov 2, 2021

Thanks @dgarros - I will look further into this as see how these changes affects DiffSync.

Comment on lines 72 to 74
structlog_float_version = float(".".join(structlog.__version__.split(".")[:2]))
if structlog_float_version < 21.2:
return True
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks pretty gross. Can we use packaging.Version objects here instead? Something like:

from packaging import version

if version.parse(structlog.__version__) < version.Version("21.2.0"):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion - I've implemented the change.

structlog.processors.StackInfoRenderer(),
structlog.processors.format_exc_info,
structlog.dev.ConsoleRenderer(),
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need a processors=processors, line added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it got lost somewhere in the experimentation phase. Added this back in.

@grelleum
Copy link
Contributor Author

The structlog change in 21.2.0 and above changes behavior with regards to the use of format_exc_info as a log processor.
This effects exception logging when using the logging.enable_console_logging function.

When using 21.2.0 or newer and either better_exceptions or rich in available in the Python environment, then exception logging will generate a warning if structlog.processors.format_exc_info is installed as a processor. If we remove the format_exc_info processor then we lose exception traceback formatting, when the above is not the case.

The code added here determines if the structlog.processors.format_exc_info is required in the processor list, and appends it to the list if required.

@grelleum
Copy link
Contributor Author

Found another issue in my code.

    rich = importlib.util.find_spec("rich")
    better_exceptions = importlib.util.find_spec("better_exceptions")

It looks like importlib.util has been removed from Python 3.9.
I am investigating the proper way to determine if a library is installed without actually importing the library.

@grelleum
Copy link
Contributor Author

For compatibility with Python 3.9, I changed:

import importlib
...
    rich = importlib.util.find_spec("rich")
    better_exceptions = importlib.util.find_spec("better_exceptions")

to

from importlib.util import find_spec
... 
    rich = find_spec("rich")
    better_exceptions = find_spec("better_exceptions")

Copy link
Contributor

@dgarros dgarros left a comment

Choose a reason for hiding this comment

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

LGTM thank you @grelleum for the PR
@glennmatthews do you think we are good to merge ?

Copy link
Collaborator

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

:shipit:

@glennmatthews glennmatthews merged commit 32bad81 into networktocode:main Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants