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: do use OBSCURE_FILENAME instead of hardcoded unicode #5944

Merged
merged 2 commits into from
Sep 10, 2021

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Sep 1, 2021

OBSCURE_UNICODE should be used since otherwise tests would
just keep crashing while running on file systems/environment
which do not support those fancy characters.

Initial intention here was to address some Windows fails on Vista and
other elderly systems. #2929
If tests pass on current CI I would assume that we are past that
stone age.

Closes a part of #5942. Wanted first to see if there are side-effects

OBSCURE_UNICODE should be used since otherwise tests would
just keep crashing while running on file systems/environment
which do not support those fancy characters.

Initial intention here was to address some Windows fails on Vista and
other elderly systems.  datalad#2929
If tests pass on current CI I would assume that we are past that
stone age.

Closes a part of datalad#5942.  Wanted first to see if there are side-effects
@yarikoptic yarikoptic added the tests Add or improve existing tests label Sep 1, 2021
@codecov
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

Merging #5944 (44aa5ab) into maint (dfdd352) will decrease coverage by 6.56%.
The diff coverage is 100.00%.

❗ Current head 44aa5ab differs from pull request most recent head 0ceb5ff. Consider uploading reports for the commit 0ceb5ff to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5944      +/-   ##
==========================================
- Coverage   90.28%   83.72%   -6.57%     
==========================================
  Files         300      297       -3     
  Lines       42516    41375    -1141     
==========================================
- Hits        38387    34642    -3745     
- Misses       4129     6733    +2604     
Impacted Files Coverage Δ
datalad/support/tests/test_globbedpaths.py 100.00% <100.00%> (ø)
datalad/support/tests/test_network.py 98.60% <100.00%> (-0.01%) ⬇️
datalad/utils.py 79.64% <100.00%> (-5.46%) ⬇️
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/metadata/extractors/tests/test_audio.py 20.00% <0.00%> (-80.00%) ⬇️
datalad/metadata/extractors/xmp.py 12.96% <0.00%> (-79.63%) ⬇️
datalad/metadata/extractors/exif.py 18.75% <0.00%> (-78.13%) ⬇️
datalad/metadata/extractors/image.py 19.35% <0.00%> (-77.42%) ⬇️
datalad/metadata/extractors/audio.py 20.00% <0.00%> (-77.15%) ⬇️
datalad/metadata/extractors/tests/test_exif.py 25.00% <0.00%> (-75.00%) ⬇️
... and 197 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 dfdd352...0ceb5ff. Read the comment docs.

@yarikoptic
Copy link
Member Author

besides codecov all green, good, so I assume that appveyor is our gold standard for minimal version of windows to be compatible and will proceed fixing other similar failures in the same fashion.

@yarikoptic
Copy link
Member Author

Windows doesn't like my questionable fix

TypeError: bytes args is not allowed on Windows

datalad/cmd.py Outdated Show resolved Hide resolved
@yarikoptic
Copy link
Member Author

Extracted a more "core" change to #5961 so this PR now concerns only with tests.

@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Sep 7, 2021
@yarikoptic yarikoptic added this to the 0.14.8 milestone Sep 7, 2021
@adswa
Copy link
Member

adswa commented Sep 9, 2021

I can't spot anything wrong with this PR, looks sensible to my not-so-expert eyes :)

@yarikoptic
Copy link
Member Author

Thank you @adswa for feedback on all PRs staged for the release. Let's cut out the release by merging this PR, adding the label

@yarikoptic yarikoptic added the release Create a release when this pr is merged label Sep 10, 2021
@yarikoptic yarikoptic merged commit ebd2042 into datalad:maint Sep 10, 2021
@yarikoptic
Copy link
Member Author

releasing failed (#5972 ), taking off the label since we might just want to retry

@yarikoptic yarikoptic removed the release Create a release when this pr is merged label Sep 10, 2021
@yarikoptic yarikoptic deleted the bf-unicode-in-tests branch October 8, 2021 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok OP considers this work done, and requests review/merge tests Add or improve existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants