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 for py-37 #807

Closed
wants to merge 2 commits into from
Closed

fix for py-37 #807

wants to merge 2 commits into from

Conversation

ali96343
Copy link
Contributor

Minor code improvements.
Added an example of writing to the server-py4web.log from the controller.
Tested with rocket,wsgiref, tornado, gevent
Fix: encoding="utf-8" for python <= 3.9

@Sukhichev
Copy link
Contributor

  1. I'm not sure that a writing to the server-py4web.log from the controller is good idea, because server-py4web.log is apps server log file not apps log file. In other words, this is not public API. Your app can write the logs to its own log file.
  2. Your example has a global variable. Please read this: https://www.readersfact.com/are-global-variables-bad-python/
  3. Also you add two global "constants" to the code. It doesn't look good either.
  4. You don't fix bug. Really you change one bug to other bug, because in you case encoding=None for python < 3.9. I.e., log file will write in a platform-dependent encoding that is different for different platforms.
  5. Your commit contains many issues. It is really hard to read. Could you split your commit to some commits where one commit is one issue?

@ali96343
Copy link
Contributor Author

Hello

1 For debugging event-driven programs
(i.e. Socketio, Tornado, SSE),
it is convenient to see the sequence and times of events in a single log file.

2.3 We can see global variables in almost any python library file

  1. My patch allows py4web to work with python version >=3.7.
    I think that's enough

5 Here is my working version server_adapters.py.

https://github.com/ali96343/lvsio/blob/main/mig1ssl/server_adapters.py

I am ready to answer any questions about this file

Thank you for your comments

@ali96343 ali96343 closed this Aug 30, 2023
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