Skip to content

Fix UnicodeDecodeError for Umlaute in translogger #74

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

2silver
Copy link

@2silver 2silver commented Mar 10, 2023

Fix for #73 - do not use %b use %m instead.

Fix for pasteorg#73 - do not use `%b` use `%m` instead.
@cdent
Copy link
Collaborator

cdent commented Mar 11, 2023

Unfortunately this fix introduces a potential compatibility issue that we need to think about before being certain we want to merge it:

If any external tooling is parsing the log message and expecting a month name, replacing it with numbers will break that parsing.

Ideas on how to deal with that?

cc @FND as they might have some insight

@FND
Copy link

FND commented Mar 11, 2023

Ouch, that seems tricky indeed...

I don't really know much about Paste's user base, nor can I quite imagine why someone would want localized logs (fully aware that's somewhat ignorant). The fact that Paste's relationship with developers is often indirect (AIUI, they might think they're using Plone, with Paste working under the covers) makes this even harder to assess.

I suppose the only safe option is to make this configurable: Given that Paste is in maintenance mode, changing behavior seems ill-advised - that leaves the opt-in route: If this was a TransLogger constructor argument, at least users (including frameworks) would have the option of fixing this issue on their end, where they have more context to make a decision.

@2silver
Copy link
Author

2silver commented Mar 11, 2023

I completely agree that localized logs are unnecessary.
However, when running the program on MacOS, the logs are translated by default - perhaps due to my messy setup. It would be helpful to have an optional configuration for this feature.
Additionally, using unicode/UTF-8 strings would be beneficial for compatibility - even with outdated versions of Python 2.

@cdent
Copy link
Collaborator

cdent commented Mar 12, 2023

@2silver are you happy to adjust the patch to have Translogger take an argument, something like numeric_month=False that if true uses %m.

My preference is to keep the extent of changes as small as possible, and an optional kwarg is probably the easiest way to do that.

@FND
Copy link

FND commented Mar 12, 2023

Any reason not to make the entire timestamp configurable while you're at it, e.g. timestamp='%d/%b/%Y:%H:%M:%S'?

@cdent
Copy link
Collaborator

cdent commented Mar 12, 2023

Any reason not to make the entire timestamp configurable while you're at it, e.g. timestamp='%d/%b/%Y:%H:%M:%S'?

That's a good idea.

Which means we end up with two options on how to do this:

  • as a passed in kwarg as defined
  • as a class variable which can be overridden in a subclass (the issue with the current code is that it is hard to easily subclass for changed behaviour: the existing method is doing too much)

@2silver
Copy link
Author

2silver commented Mar 17, 2023

I tried it with this commit 0db6e41

@cdent
Copy link
Collaborator

cdent commented Apr 30, 2023

Sorry for not getting back to you sooner about this. I've unfortunately not yet had time to give this a proper review. A quick glance shows that the changes a conceptually okay, but some of the details of the Python code need some work. Depending on your preference I can help you to push it in the right direction, once I get time to give it a proper review, or I can take the work you've started and change it myself. What's your preference?

@cdent
Copy link
Collaborator

cdent commented Aug 26, 2023

My apologies or taking such a very long time to look at this. I've gone ahead and don't a different revision in #80 . If you could try that and let me know if it works for your purposes I'll go ahead and merge it and make a new release.

The concepts in my implementation are the same as yours, but done in a simpler fashion.

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