Skip to content

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented May 2, 2020

No description provided.

@codecov
Copy link

codecov bot commented May 2, 2020

Codecov Report

Merging #57 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #57   +/-   ##
=======================================
  Coverage   97.60%   97.60%           
=======================================
  Files           2        2           
  Lines         125      125           
  Branches       31       31           
=======================================
  Hits          122      122           
  Misses          1        1           
  Partials        2        2           
Impacted Files Coverage Δ
src/unasync/__init__.py 97.58% <100.00%> (ø)

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.

Looks good to me

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

One thought, otherwise LGTM


return False

def unasync_file(self, filepath):
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I actually use this API at work because it's a part of the existing code generator. All others I agree are private.

Something I've been thinking of is unasync should have a simple CLI, that'd fix the issue of not wanting to use it for a build system.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting! Can you share more about the use case?

Yeah, I guess a cli would be nice. For hip, I plan to use hip from nox, so a Python interface is enough.

I marked Rule.unasync_file as public, please take another look.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also use it from the Python interface: https://github.com/elastic/elasticsearch-py/pull/1203/files#diff-11742648bf5996c880c7a08632da8eddR305

Maybe I can put together something for CLI, I can see it being useful for people using tox and pre-commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, awesome! Sure, a cli would be useful, just not to us right now

@pquentin pquentin merged commit cfd7c7f into python-trio:master May 2, 2020
@pquentin pquentin deleted the public-api branch May 2, 2020 17:01
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