Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants