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

logging: improve default logging format (issue5214) #5227

Merged
merged 1 commit into from
May 8, 2019

Conversation

Pulkit07
Copy link
Contributor

@Pulkit07 Pulkit07 commented May 7, 2019

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
Copy link
Contributor Author

Pulkit07 commented May 7, 2019

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

Copy link
Member

@nicoddemus nicoddemus left a comment

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.

changelog/5214.logging.rst Outdated Show resolved Hide resolved
src/_pytest/logging.py Show resolved Hide resolved
@blueyed
Copy link
Contributor

blueyed commented May 7, 2019

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
Copy link

codecov bot commented May 8, 2019

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
Copy link
Contributor Author

Pulkit07 commented May 8, 2019

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

Copy link
Member

@nicoddemus nicoddemus left a comment

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
Copy link
Contributor Author

Pulkit07 commented May 8, 2019

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
Copy link
Contributor

blueyed commented May 8, 2019

#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
Copy link
Contributor

blueyed commented May 8, 2019

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