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

draft: --add-timestamp #323

Merged
merged 3 commits into from
Jun 18, 2020
Merged

draft: --add-timestamp #323

merged 3 commits into from
Jun 18, 2020

Conversation

omry
Copy link
Contributor

@omry omry commented May 9, 2020

introducing an --add_timestamp flag and support for in in the logging system.

#289

@omry
Copy link
Contributor Author

omry commented May 9, 2020

  1. Looking for overall feedback (Is it okay to add the flag? is the log format okay? [I noticed it's missing nox> )
  2. colorlog test is failing even though things are working. looks like some kind of interaction between colorlog and logcap.
    I can probably test it via a process exec or something if it's very important.

@omry
Copy link
Contributor Author

omry commented May 9, 2020

Screen shots:

image
image

@omry
Copy link
Contributor Author

omry commented May 25, 2020

@theacodes ping

Copy link
Collaborator

@theacodes theacodes left a comment

Choose a reason for hiding this comment

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

Looks like a great addition, one little comment.

nox/_options.py Outdated Show resolved Hide resolved
@theacodes
Copy link
Collaborator

Looking for overall feedback (Is it okay to add the flag? is the log format okay? [I noticed it's missing nox> )

This is a great idea. I would definitely add the nox >, as it's important to be able to distinguish Nox's output from the output that programs called via run produce.

colorlog test is failing even though things are working. looks like some kind of interaction between colorlog and logcap.

We'll definitely wanna get that working, but I'm not a fan of using subprocess in tests unless its absolutely necessary.

@omry
Copy link
Contributor Author

omry commented May 26, 2020

We'll definitely wanna get that working, but I'm not a fan of using subprocess in tests unless its absolutely necessary.

The problem I ran into was that for an unknown reason, asctime was not populated in the caplog record when using colorlog. but we are talking about an interaction between two third party libs (caplog and colorlib), through the somewhat complicated Python logging subsystem.

I already spent about an hour or two digging into it. I will give it another shot before resorting to process testing.

@omry omry changed the title draft: --add_timestamp draft: --add-timestamp May 26, 2020
@omry
Copy link
Contributor Author

omry commented May 26, 2020

responded to comments, except the test.
Upon further digging, I suspect custom log formatter are not fully supported by pytest caplog.

I see two options:

  1. Drop the testing for timestamp+colorlog and keep the current test for non-colorlog mode.
    This is very visible so if it breaks we will hear about it.
  2. Switch both test modes to process based testing (which can be fragile, given we are testing for the presence of a timestamp in the log output).

What do you think?

@theacodes
Copy link
Collaborator

Sorry for taking so long to get to this. Let's go with (1).

@omry
Copy link
Contributor Author

omry commented Jun 18, 2020

Regular run

image

Run with timestamps

image

Run with timestamp and no color.

image

@omry omry marked this pull request as ready for review June 18, 2020 06:49
@omry
Copy link
Contributor Author

omry commented Jun 18, 2020

I noticed that there is some cleanup to do:

  1. blacken is not running and seems to be producing results different than black .. it should probably be removed from the noxfile.
  2. docs/conf.py needs black formatting.

@theacodes theacodes merged commit 8dcefbf into wntrblm:master Jun 18, 2020
@theacodes
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants