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

Fix spelling of parameter json_serialiser -> json_serializer #8

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

juliangilbey
Copy link
Contributor

@juliangilbey juliangilbey commented Mar 29, 2024

The parameter json_serializer of the JsonFormatter class is publicised and used by other packages when importing and using this package. So its name cannot be changed to json_serialiser. This PR reverts this (possibly inadvertent) change.

@juliangilbey
Copy link
Contributor Author

One further thought - if you really do want to allow the UK spelling, you could do that as an alternative, something like the following:

    def __init__(
        ...,
        json_serialiser: Union[Callable, str] = json.dumps,
        json_serializer: Union[Callable, str] = json.dumps,
        ...
    ) -> None:
        """
        ...
        :param json_serialiser: a :meth:`json.dumps`-compatible callable
            that will be used to serialize the log record.
        :param json_serializer: an alternative spelling for :param:`json_serialiser`.
        ...
        """
        ...
        if json_serialiser != json.dumps:
            self.json_serializer = self._str_to_fn(json_serialiser)
        else:
            self.json_serializer = self._str_to_fn(json_serializer)
        ...

@nhairs
Copy link
Owner

nhairs commented Apr 1, 2024

Thanks for finding this.

So it looks like this change occurred during 5f85723 when the constructor was changed from **kwags to actual key-word arguments. The "fix" (madzak/python-json-logger#170) was included on the main branch of the upstream but never released, thus it was inadvertently released in 3.0.0.

I'll release it as a patch release (3.0.1)

One further thought - if you really do want to allow the UK spelling, you could do that as an alternative, something like the following:

The library is already using US spelling, lets stick with it :)

@nhairs nhairs merged commit 0e1ff25 into nhairs:main Apr 1, 2024
31 checks passed
@juliangilbey
Copy link
Contributor Author

Thanks for finding this.

The pleasure of having Debian's amazing CI system; when I uploaded 3.0.0, all of the packages depending on it had their package tests run against the new version!

Thanks for fixing it so quickly, too.

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.

2 participants