[Simplified] Move pytest-logging functionality to pytest-catchlog #33
Conversation
| By default, catchlog will output any logging records with a level higher or equal | ||
| to WARNING. In order to actually see these logs in the console you have to disable | ||
| pytest output capture by passing ``-s``. The life span of the CLI logging handler | ||
| lasts throughout the whole runtests session. |
There was a problem hiding this comment.
nitpick: I'd just say "the whole test session" here, as someone not familiar with pytest internals might not know what you mean with "runtests session".
Alternatively maybe get rid of the whole sentence? This sounds like an implementation detail which is irrelevant to a catchlog user anyways, no?
There was a problem hiding this comment.
I'll get rid of the sentence.
|
What's about adding a |
|
I didn't initially add However, I see no harm, and its actually beneficial, to add the fallback support you suggest. I'll ping you guys once I'm finished addressing your suggestions... |
|
What I mean with it would suggest is that when users read the whole CLI help message and the available option args, they'd notice the CLI/file distinction and would come to that conclusion? |
|
PR updated. |
That's true indeed, though #24 fixes that, i.e. tests will always use the standard formatting, regardless command line options. |
|
Ok! Cool! |
|
So, after #24 is merged, we remove the CLI specific options and use the original ones? |
|
I thought you propose CLI options to be used for live logs, don't you? In this sense, are file options that different from CLI ones to remove the latter, but leave the former? |
|
Yes, CLI options are for live logs, however, I personally, in a CI context, want live logs less verbose and file logs more verbose, hence the separation. |
|
BTW, how is a log file supposed to be used on CI? |
|
We use Jenkins and we store it as an artifact for later inspection if need be. |
|
Is there anything that you'd like me to address on this PR? |
|
Honestly, I doubt the need to add a new set of command line options ( Is it possible to get along with adding just the |
|
The problem is not the log level option, the problem is the format options which already exist and are not targeted towards CLI usage, they are targeted towards test usage. When that stops being the case, after your other PR is merged, then we can drop the CLI options and reuse the existing ones. So, either this goes in with the CLI options to avoid confusion, or, that other PR is merged first and the options are reused fotncli usage, since they won't be targeted for test usage anymore. Did I make any sense? |
Yeah, totally. I'll try to address remaining issues of that PR to get it merged then. |
|
Awesome! |
|
Rebased to resolve conflicts |
|
Where are we on this PR? I think I need to rebase... Let me know of anything else required. |
|
I've solved conflicts again. Whats the status of this PR? |
|
I apologize this PR got stalled. The thing is, I have a new job, hence no spare time now, alas. @eisensheng @The-Compiler we need to get over with this feature, I feel ashamed =( |
|
No need to feel ashamed. I know about being busy, why do you think I haven't asked a month or two ago? 😄 |
|
I agree we somehow need to get all the open stuff in (and then maybe talk about the rename to I've looked at this PR again, and it seems fine to me. I'm somewhat inclined to just merge it - is there anything still open holding this up? |
|
Nothing holding on my end and about the rename, just say when, although, I'd like to have my other PR merged in too before(which I'll have to rebase once this one is merged). |
|
I'm also very sorry regarding the current state of this issue. I planned to get back to this issue on this weekend but wasn't able to make it in the end unfortunately. What's left to do here for me is basically to review the code and the use cases over again, running the test suite, inspecting coverage and (hopefully) approving it. I scheduled the coming saturday for this. |
|
👍 |
| def get_actual_log_level(config, setting_name): | ||
| ''' | ||
| Return the actual logging level | ||
| ''' |
There was a problem hiding this comment.
Can you use double quotes for docstrings, please?
Please look at the bottom of https://www.python.org/dev/peps/pep-0257/#id15 for reference.
There was a problem hiding this comment.
I know that pep, i just choose not to follow for my own projects, I prefer single quotes.
I'll fix it in a bit.
There was a problem hiding this comment.
If we want to follow pep257, that should be """Return the actual logging level.""" - all in one line and with a period 😉
| @@ -50,12 +56,60 @@ def pytest_addoption(parser): | |||
| dest='log_date_format', default=DEFAULT_LOG_DATE_FORMAT, | |||
| help='log date format as used by the logging module.' | |||
| ) | |||
| add_option_ini( | |||
There was a problem hiding this comment.
It's not clear to me why there are two different options that appear to configure the same thing. Can you enlighten me here, please?
There was a problem hiding this comment.
There already was some discussion about this.
There was a problem hiding this comment.
Duh! I looked through usage in source but missed that discussion. Sorry!
|
@s0undt3ch What do you think about adding a test that actually tries to log a message and will appear on stderr accordingly? It might be a bit redundant but that test would be bound to the feature implemented here. Otherwise it looks good to me apart from that one docstring nitpick and would like to merge it sooner than later. I'm sorry if my tone was to harsh, it doesn't look to me like that but I was reading some (amusing) torvalds mails before reviewing the branch and I'm afraid his tone slipped in. |
|
No worries about the tones, we all want the same goal, an excellent tool, and sometimes we just see some awkward code which brings out the tone(personal experience), or sometimes the goal is just not properly explained. I'll add that test later in the evening or as soon as possible, I also want this in sooner than latter. |
|
Single quote replaced with double quotes and tests updated to ensure we actually get the expected logging output in |
|
Wheee! ✨ |
|
This feature was merged on top of several more changes. It's difficult to draw a line there but the clean up of the entire plugin justifies the next version number to be 2.0.0 but there is still work left to do for the 2.0.0 release. Among that is the pytest-logging rename. This means it will take a little bit longer until this feature hits a published version. Is this okay for anyone interested in this feature or shall I do a 1.x release containing this feature beforehand? |
|
Woohooo!!!! About waiting for 2.0, no worries on my end, I can keep installing from git. |
This is a simplified and a more explicit approach to #20
Closes #20.