Skip to content

Improve print-debugging (no flush=True required; require -d/--debug) #1122

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 21, 2021
Merged

Improve print-debugging (no flush=True required; require -d/--debug) #1122

merged 3 commits into from
Aug 21, 2021

Conversation

neiljp
Copy link
Collaborator

@neiljp neiljp commented Aug 17, 2021

What does this PR do?

This mainly improves print debugging:

  • just use print("something") instead of print("something", flush=True) (README updated)
  • only open the debug.log file that print is redirected to, if -d or --debug is enabled (README updated)

There is also an intermediate refactor commit to require Controller to have named arguments.

Fixes #1066 (via the second part)

Tested?

  • Manually
  • Existing tests (adapted, if necessary)
  • New tests added (for any new behavior)
  • Passed linting & tests (each commit)

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Aug 17, 2021
@neiljp neiljp added this to the Next Release milestone Aug 17, 2021
@neiljp neiljp changed the title WIP Improve print-debugging (no flush=True required; require -d) Improve print-debugging (no flush=True required; require -d/--debug) Aug 17, 2021
@neiljp neiljp changed the title Improve print-debugging (no flush=True required; require -d/--debug) Improve print-debugging (no flush=True required; require -d/--debug) Aug 17, 2021
@neiljp neiljp added area: documentation Requires changes in documentation area: infrastructure Project infrastructure labels Aug 17, 2021
zee-bit
zee-bit approved these changes Aug 19, 2021
View reviewed changes
Copy link
Member

@zee-bit zee-bit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this done, Neil. 👍
Apart from the minor point raised by Preet on the stream, this looks good to me.

@neiljp
README/run/core: Only create & log to debug.log if debugging is enabled.
c7a7ace 
To centralize the specification of the logging file, this is now set in
`run.py` and passed into the controller as an `Optional[str]` argument.

Tests amended.

Fixes zulip#1066.
@neiljp neiljp force-pushed the 2021-08-17-remove-need-for-print-flush branch from 067374d to c7a7ace Compare August 21, 2021 01:48
@neiljp
Copy link
Collaborator Author

neiljp commented Aug 21, 2021

@zee-bit Thanks for the feedback!
@preetmishra Thanks for the suggestion to output the debug.log info when starting 👍

I've adjusted for that and merging now - let's get to using this! :)

@neiljp neiljp merged commit 5f71475 into zulip:main Aug 21, 2021
@neiljp neiljp mentioned this pull request Aug 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@zee-bit zee-bit zee-bit approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area: documentation Requires changes in documentation area: infrastructure Project infrastructure size: L [Automatic label added by zulipbot]
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

add option to disable debug.log
3 participants
@neiljp @zee-bit @zulipbot