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+ENH(TST): fix typo in code of wtf filesystems reports #6920

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Aug 5, 2022

Detected while looking at #6917 (comment) and it was impossible to detect that there is an underlying issue in the code since no logging and swallowing of all exceptions silently. Added logging, and some rudimentary testing (let's see if works on windows. edit: appveyor was green, so I guess it did!).

@yarikoptic yarikoptic requested a review from adswa August 5, 2022 01:18
@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #6920 (06f50e0) into maint (06f50e0) will not change coverage.
The diff coverage is n/a.

❗ Current head 06f50e0 differs from pull request most recent head 9c9c34c. Consider uploading reports for the commit 9c9c34c to get more accurate results

@@           Coverage Diff           @@
##            maint    #6920   +/-   ##
=======================================
  Coverage   90.03%   90.03%           
=======================================
  Files         354      354           
  Lines       46246    46246           
  Branches     6595     6595           
=======================================
  Hits        41637    41637           
  Misses       4593     4593           
  Partials       16       16           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Aug 5, 2022
Even if it would be too noisy on windows -- would still be better than
just silently not working correctly (as it was due to a typo).

And if psutil is not installed, we would get it logged only at debug level. But
otherwise, if something abnormal -- at warning level and reported once
(although could be different for different filesystems, but at least would
alert to a possible next typo)
@yarikoptic
Copy link
Member Author

FWIW I have CPed the actual fix (b5fd8cd) into maint since there is nothing to discuss there. Will keep this PR for review of the rest (testing etc).

Copy link
Member

@adswa adswa left a comment

Choose a reason for hiding this comment

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

Thanks for catching this, and adding a test. And sorry for the vacation-related delay

@yarikoptic yarikoptic merged commit 8fec36a into datalad:maint Aug 15, 2022
@yarikoptic yarikoptic deleted the bf-wtf-filesystem branch October 14, 2022 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants