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

Add Caddy json log support #367

Merged
merged 3 commits into from
Jan 31, 2024
Merged

Conversation

simon04
Copy link
Contributor

@simon04 simon04 commented Dec 24, 2023

Description:

Add Caddy json log support, https://caddyserver.com/docs/caddyfile/directives/log

Review

@michalkleiner
Copy link
Contributor

michalkleiner commented Dec 28, 2023

Hi @simon04, thank you for taking the time and opening the PR adding the Caddy log support.
I'm not a Caddy user myself but I was wondering if the PR change could be done without adding the types? Is that required for it to work or could we do without the typings? It seems the most risky part of the change as no other format import uses that.

@simon04
Copy link
Contributor Author

simon04 commented Jan 26, 2024

Hi @michalkleiner, the types are defined in https://peps.python.org/pep-0484/ and are being used more and more in the Python ecosystem. Sure, the code works w/o types as well, but we'd loose some static analysis capabilities.

Since the type classes are wrapped in typing.TYPE_CHECKING, no harm should be done for production:

A special constant that is assumed to be True by 3rd party static type checkers. It is False at runtime.
https://docs.python.org/3.12/library/typing.html#typing.TYPE_CHECKING

Please let me know how to proceed...

@michalkleiner
Copy link
Contributor

@sgiehl what's your opinion here? I'd accept the PR without the types, but if you're happy with it as is then all good with me.

@sgiehl
Copy link
Member

sgiehl commented Jan 29, 2024

@simon04 Yes. Please remove the typing for now. We can consider adding that at a later step for the whole script if useful.
Please also rebase your branch on the latest changes from 5.x-dev, hopefully this will trigger the tests to run for this PR.

@simon04
Copy link
Contributor Author

simon04 commented Jan 30, 2024

Hi @michalkleiner and @sgiehl, I've made the progress in static type checking undone. Feel free to merge now.

@michalkleiner michalkleiner merged commit d1cd2fe into matomo-org:5.x-dev Jan 31, 2024
6 checks passed
@michalkleiner
Copy link
Contributor

Thank you for the contribution, @simon04!

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.

3 participants