Skip to content

logging: improve default logging format (issue5214)#5227

Merged
blueyed merged 1 commit into
pytest-dev:featuresfrom
Pulkit07:issue5214
May 8, 2019
Merged

logging: improve default logging format (issue5214)#5227
blueyed merged 1 commit into
pytest-dev:featuresfrom
Pulkit07:issue5214

Conversation

@Pulkit07

@Pulkit07 Pulkit07 commented May 7, 2019

Copy link
Copy Markdown
Contributor

We improve the following things in the logging format:

  • Show module name instead of just the filename
  • show level of logging as the first thing
  • show lineno attached to module:file details

Thanks to @blueyed who suggested this on the github issue.

It's my first contribution and I have added myself to AUTHORS.

I also added to a changelog file.

Fix #5214

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Target the master branch for bug fixes, documentation updates and trivial changes.
  • Target the features branch for new features and removals/deprecations.
  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS in alphabetical order;

@Pulkit07

Pulkit07 commented May 7, 2019

Copy link
Copy Markdown
Contributor Author

Tests are failing on the filename of the changelog file added. Shall I rename that to 5214.bugfix.rst instead of 5214.logging.rst?

@nicoddemus nicoddemus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @Pulkit07,

Thanks a ton for your contribution, we greatly appreciate it!

As this is changing an existing behavior, this should go into the features branch, so please rebase on top of features. 👍

Also please take a look at my comments and let me know what you think.

Comment thread changelog/5214.logging.rst Outdated
Comment thread src/_pytest/logging.py
Comment thread changelog/5214.logging.rst Outdated
@blueyed

blueyed commented May 7, 2019

Copy link
Copy Markdown
Contributor

Thanks already!

rebase on top of features

You can force-push here, and then change the target branch at the top next to the PR title.

While at it, please also edit the commit message (i.e. remove speculation about having a changelog from there).. :)

@Pulkit07 Pulkit07 changed the base branch from master to features May 8, 2019 18:25
@codecov

codecov Bot commented May 8, 2019

Copy link
Copy Markdown

Codecov Report

Merging #5227 into features will decrease coverage by 1.49%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           features    #5227     +/-   ##
===========================================
- Coverage     96.13%   94.63%   -1.5%     
===========================================
  Files           115      115             
  Lines         26040    26040             
  Branches       2568     2568             
===========================================
- Hits          25033    24643    -390     
- Misses          706     1080    +374     
- Partials        301      317     +16
Impacted Files Coverage Δ
testing/logging/test_reporting.py 100% <ø> (ø) ⬆️
src/_pytest/logging.py 95% <100%> (ø) ⬆️
testing/test_argcomplete.py 20.28% <0%> (-47.83%) ⬇️
src/_pytest/_argcomplete.py 33.33% <0%> (-41.67%) ⬇️
testing/python/approx.py 80.45% <0%> (-19.18%) ⬇️
src/_pytest/python_api.py 87.44% <0%> (-10.05%) ⬇️
testing/test_pdb.py 89.77% <0%> (-9.42%) ⬇️
testing/test_conftest.py 90.4% <0%> (-9.23%) ⬇️
testing/test_pathlib.py 91.17% <0%> (-8.83%) ⬇️
src/_pytest/pytester.py 85.53% <0%> (-6.1%) ⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2051e30...7e08e09. Read the comment docs.

We improve the following things in the logging format:

  * Show module name instead of just the filename
  * show level of logging as the first thing
  * show lineno attached to module:file details

Thanks to @blueyed who suggested this on the github issue.

It's my first contribution and I have added myself to AUTHORS.

I also added to a changelog file.
@Pulkit07

Pulkit07 commented May 8, 2019

Copy link
Copy Markdown
Contributor Author

Hey @nicoddemus and @blueyed, thanks for such a quick review.

@nicoddemus nicoddemus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome, thanks a lot @Pulkit07 for the contribution! 👍

@Pulkit07

Pulkit07 commented May 8, 2019

Copy link
Copy Markdown
Contributor Author

Awesome, thanks a lot @Pulkit07 for the contribution!

Thanks to you too. Do you have any starter bug in mind which I can work on?

Also, I am unable to find some mention about IRC channel or gitter or something where discussions related to pytest happen. Most probably I missed something. Can you help me pointing at any such channel which I can join and take help in resolving future issues?

@blueyed

blueyed commented May 8, 2019

Copy link
Copy Markdown
Contributor

#pylib on Freenode (IRC)

You have not changed the commit message (try git commit --amend).

While it would be ok, I got like a lot of notifications due to me being mentioned therein, and I'd rather not get more of them - i.e. it is usually a bad idea to mention somebody in commit messages.
Typically you would do so in the PR comment then - the same for asking about if a changelog is needed etc.

@blueyed blueyed merged commit ed2b715 into pytest-dev:features May 8, 2019
@blueyed

blueyed commented May 8, 2019

Copy link
Copy Markdown
Contributor

btw: normally you could just edit/remove this when squash-merging, but other maintainers do not like it unfortunately.

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