Skip to content

Conversation

sethmlarson
Copy link
Contributor

This PR adds a new feature called Rule to unasync which allows configuring unasync to act differently in different directories. The rule with the most specific match to a given path applies, allowing for additional replacements that only affect certain sub-directories.

I maintained the current API with unasync_X() functions deriving from a "default" rule of _async -> _sync.

@codecov
Copy link

codecov bot commented Feb 5, 2020

Codecov Report

Merging #55 into master will increase coverage by 0.63%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #55      +/-   ##
=========================================
+ Coverage   96.96%   97.6%   +0.63%     
=========================================
  Files           4       4              
  Lines         198     250      +52     
  Branches       46      62      +16     
=========================================
+ Hits          192     244      +52     
  Misses          2       2              
  Partials        4       4
Impacted Files Coverage Δ
src/unasync/__init__.py 97.58% <100%> (+0.64%) ⬆️
unasync/__init__.py 97.58% <0%> (+0.64%) ⬆️

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

This is really solid work, thanks! I've left a few comments but nothing is blocking so feel free to merge this as-is.



def customize_build_py(rename_dir_from_to=("_async", "_sync")):
def customize_build_py(rules=(_DEFAULT_RULE,)):
Copy link
Member

Choose a reason for hiding this comment

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

This is not specific to this PR, but I'm not too happy that we have both build_py and customize_build_py. I think we can transform customize_build_py into build_py.__call__, and possibly only expose build_py and Rule as part of our API. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I can make that change in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm so build_py is a class so we can't use __call__ unfortunately. Maybe we can use cmdclass_build_py(rules=[]) and make the build_py class a part of the private interface?

Copy link
Member

@RatanShreshtha RatanShreshtha 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 we should also document the changes and update the usage example in README.md

@sethmlarson
Copy link
Contributor Author

Updated this PR per comments, thanks all! Would love another review. :)

@pquentin
Copy link
Member

Thanks! Unfortunately the build breaks because of the virtualenv mess

@sethmlarson
Copy link
Contributor Author

@pquentin Is there an issue somewhere I can track? There's multiple things blocked on that. :/

@pquentin
Copy link
Member

@sethmlarson Yes that's https://travis-ci.community/t/python-development-versions-no-longer-include-pip-due-to-virtualenv-20-x/7180

Feel free to allow failures in the 3.8-dev build for now if that can unblock you

@pquentin
Copy link
Member

Cycling, hopefully the Travis issue is now fixed.

@pquentin pquentin closed this Feb 13, 2020
@pquentin pquentin reopened this Feb 13, 2020
@pquentin pquentin merged commit 6b6ca3b into python-trio:master Feb 13, 2020
@pquentin
Copy link
Member

Thank you @sethmlarson!

@sethmlarson sethmlarson deleted the rules branch February 13, 2020 13:43
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