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

RF: de-wrapt #6190

Merged
merged 7 commits into from Nov 16, 2021
Merged

RF: de-wrapt #6190

merged 7 commits into from Nov 16, 2021

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Nov 13, 2021

A quick chat with @mih inspired me to look into it.... in a nutshell (more to come if feasible):

oh well -- let's see if anything else breaks ;-)

…ncs with kwonlys

Also RFed to not just warn if kwonlys found but not requested explicitly
to be included via include_kwonlyargs, but to raise ValueError similar
to how inspect.getarg*spec behaves
@yarikoptic yarikoptic added the semver-minor Increment the minor version when merged label Nov 13, 2021
apparently wrapt did smth fancy to keep access to __doc__, so we defer
binding of __doc__ composed for __call__ all the way to @build_doc
@yarikoptic
Copy link
Member Author

Woohoo, not yet sure what docs are complaining about, but the rest seems to be happy, so it might be viable

@mih
Copy link
Member

mih commented Nov 13, 2021

YES! I knew it. This is really cool. Thanks for taking it on!

@yarikoptic
Copy link
Member Author

Ha - that test said fails on Windows and Mac so may be it wasn't really wrapt to blame!

======================================================================
ERROR: datalad.runner.tests.test_nonasyncrunner.test_popen_invocation
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python39-x64\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "c:\projects\datalad\datalad\tests\utils.py", line 856, in _wrap_with_tempfile
    return t(*(arg + (filename,)), **kw)
  File "c:\projects\datalad\datalad\tests\utils.py", line 856, in _wrap_with_tempfile
    return t(*(arg + (filename,)), **kw)
  File "c:\projects\datalad\datalad\runner\tests\test_nonasyncrunner.py", line 180, in test_popen_invocation
    fetching_data.start()
  File "C:\Python39-x64\lib\multiprocessing\process.py", line 121, in start
    self._popen = self._Popen(self)
  File "C:\Python39-x64\lib\multiprocessing\context.py", line 224, in _Popen
    return _default_context.get_context().Process._Popen(process_obj)
  File "C:\Python39-x64\lib\multiprocessing\context.py", line 327, in _Popen
    return Popen(process_obj)
  File "C:\Python39-x64\lib\multiprocessing\popen_spawn_win32.py", line 93, in __init__
    reduction.dump(process_obj, to_child)
  File "C:\Python39-x64\lib\multiprocessing\reduction.py", line 60, in dump
    ForkingPickler(file, protocol).dump(obj)
TypeError: cannot pickle '_thread.lock' object

@yarikoptic yarikoptic marked this pull request as ready for review November 15, 2021 16:24
@yarikoptic
Copy link
Member Author

ok, dropped that last TEMP optimistic commit (so test will remain skipped on OSX and windows). Fixed up docstring so it builds. Should be ready for tries/review/ and even merge unless some oddity detected

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #6190 (e5505ac) into master (d8270f6) will decrease coverage by 0.65%.
The diff coverage is 87.89%.

❗ Current head e5505ac differs from pull request most recent head 95ac32f. Consider uploading reports for the commit 95ac32f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6190      +/-   ##
==========================================
- Coverage   89.25%   88.60%   -0.66%     
==========================================
  Files         317      319       +2     
  Lines       41366    41699     +333     
==========================================
+ Hits        36920    36946      +26     
- Misses       4446     4753     +307     
Impacted Files Coverage Δ
datalad/interface/__init__.py 100.00% <ø> (ø)
datalad/interface/add_archive_content.py 0.00% <0.00%> (-87.61%) ⬇️
datalad/interface/clean.py 0.00% <0.00%> (-83.34%) ⬇️
datalad/support/external_versions.py 94.87% <ø> (ø)
datalad/local/clean.py 82.85% <82.85%> (ø)
datalad/tests/test_utils.py 96.07% <84.84%> (-0.69%) ⬇️
datalad/distribution/dataset.py 96.26% <85.71%> (-0.38%) ⬇️
datalad/local/add_archive_content.py 87.98% <87.98%> (ø)
datalad/customremotes/archives.py 49.49% <100.00%> (ø)
datalad/interface/base.py 89.51% <100.00%> (+0.05%) ⬆️
... and 94 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 d8270f6...95ac32f. Read the comment docs.

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

I think this should fly. Let's try it it! Thx!

@mih mih merged commit a902b08 into datalad:master Nov 16, 2021
@yarikoptic yarikoptic deleted the rf-dewrapt branch November 17, 2021 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants