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

BF: lgr - use .setLevel() instead of .level = #3935

Merged
merged 1 commit into from Dec 16, 2019

Conversation

yarikoptic
Copy link
Member

Apparently .level is not even a property and I guess starting from
3.7 treatment has changed, or broke. May be smth like (scheduled for 3.9
with backport requests for 3.8) python/cpython#16325
was intended to fix it, I did not investigate deep enough -- just fixed
the issue we started to experience (Closes #3545)

After merged into 0.11.x, will send PR against master

Apparently .level is not even a property and I guess starting from
3.7 treatment has changed, or broke.  May be smth like (scheduled for 3.9
with backport requests for 3.8)
python/cpython#16325
was intended to fix it, I did not investigate deep enough -- just fixed
the issue we started to experience (Closes datalad#3545)
@kyleam
Copy link
Contributor

kyleam commented Dec 16, 2019

just fixed the issue we started to experience (Closes #3545)

Running a test that I know was problematic earlier, I can confirm that this quiets it. Thanks.

@kyleam
Copy link
Contributor

kyleam commented Dec 16, 2019

After merged into 0.11.x, will send PR against master

FWIW my opinion is that the extra PR is unnecessary. A local merge and push is fine unless that merge is particularly involved (and even in those cases, I think pushing to a scratch branch to make sure the tests passes is usually sufficient).

@yarikoptic
Copy link
Member Author

Ack

@yarikoptic
Copy link
Member Author

FTR: bug is interesting in that it manifested itself only after 2nd use of swallow_logs

@kyleam
Copy link
Contributor

kyleam commented Dec 16, 2019

FTR: bug is interesting in that it manifested itself only after 2nd use of swallow_logs

Weird. I remember that when I looked into it a bit when reporting, I couldn't figure out why the issue didn't seem to affect some tests. That probably explains it.

@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #3935 into 0.11.x will increase coverage by 8.77%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.11.x    #3935      +/-   ##
==========================================
+ Coverage   81.14%   89.91%   +8.77%     
==========================================
  Files         256      253       -3     
  Lines       34050    34045       -5     
==========================================
+ Hits        27629    30611    +2982     
+ Misses       6421     3434    -2987
Impacted Files Coverage Δ
datalad/utils.py 89.7% <100%> (+24.51%) ⬆️
datalad/customremotes/main.py 26.92% <0%> (-51.93%) ⬇️
datalad/customremotes/datalad.py 40.29% <0%> (-38.81%) ⬇️
datalad/customremotes/archives.py 63% <0%> (-16.19%) ⬇️
datalad/support/sshrun.py 82.5% <0%> (-7.5%) ⬇️
datalad/support/cookies.py 77.14% <0%> (-7.15%) ⬇️
datalad/downloaders/tests/test_http.py 88.94% <0%> (-3.44%) ⬇️
datalad/ui/dialog.py 88.82% <0%> (-1.18%) ⬇️
datalad/config.py 98.41% <0%> (-0.4%) ⬇️
datalad/distribution/dataset.py 91.92% <0%> (ø) ⬆️
... and 72 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 fd38a1b...f930977. Read the comment docs.

@kyleam
Copy link
Contributor

kyleam commented Dec 16, 2019

Before and after. Much better. Thanks @yarikoptic.

@yarikoptic yarikoptic merged commit a53a87c into datalad:0.11.x Dec 16, 2019
@yarikoptic
Copy link
Member Author

coolio, pushed master with 0.11.x (just this PR) merged

@yarikoptic yarikoptic deleted the bf-log-3.7 branch December 16, 2019 19:56
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.

None yet

2 participants