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

added async callable sinks and tests for async callable sinks #294

Merged
merged 6 commits into from
Jul 17, 2020
Merged

added async callable sinks and tests for async callable sinks #294

merged 6 commits into from
Jul 17, 2020

Conversation

jessekrubin
Copy link
Contributor

Would be super nice to merge this in!

Also in contributing, would be nice if you had a 'format'/'lint' script so it is easier to contribute!

What formatter are you (@Delgan) using?

@Delgan
Copy link
Owner

Delgan commented Jul 14, 2020

Hey @jessekrubin, thanks for the PR. This looks like a good idea!

I noticed that the __call__ attribute is not read-only. Consequently, there is one possible edge case (very contrived example):

def foo():
    pass

async def afoo():
    pass

foo.__call__ = afoo

logger.add(foo)

In such case, the handler is going to end up in a broken state. It expects the sink to return a coroutine object, but calling sink() will execute the synchronous foo() function. I don't expect anyone to ever change __call__ this way, but to prevent that from even being possible, what do you think of wrapping sink.__call__ instead of sink directly?

Also, it's possible that accessing the __call__ results in an AttributeError so I would like to handle that with getattr().

elif inspect.iscoroutinefunction(sink) or inspect.iscoroutinefunction(getattr(sink, "__call__", None)):
    ...

    if inspect.iscoroutinefunction(sink):
        coro = sink
    else:
        coro = sink.__call__

    wrapped_sink = AsyncSink(coro, loop, error_interceptor)

Also I don't think we need to copy/paste the whole test_coroutine_sink.py to test_async_callable_sink.py. It's a bloated enough file that I would prefer not to maintain twice. 😄

Adding a test_async_callable_object() similar to test_coroutine_function() should be enough, as well as the special cases I mentioned.

About formatting: I don't require any particular style for contributors. I run black -l 100 . myself from time to time. You're right, it's not documented. I'll add it to the tox.ini file.

@jessekrubin
Copy link
Contributor Author

Howdy @Delgan I am down with all of your suggestions (it is your package(!) (and a mighty fine one at that!)).

I get the not wanting to maintain 2 files.

Idk about adding it to tox (I think tox is a whole ordeal), I just was curious what you were doing for formatting as some people are very particular.

…ge case -- formatted src and tets with black -l 100
…all__ edge case -- formatted src and tets with black -l 100"

This reverts commit ea36950.
@Delgan
Copy link
Owner

Delgan commented Jul 14, 2020

Thanks for the update! Actually, some files under tests/ are not (yet) converted to black style (which broke comparisons between source and formatted exception), sorry you had to revert your commit. 😅

However, I'm still wondering... Why are the entire _logger.py and test_coroutine_sink.y files marked as changed in the PR diff, instead of just the few lines modified: https://github.com/Delgan/loguru/pull/294/files ? 😕

@jessekrubin
Copy link
Contributor Author

However, I'm still wondering... Why are the entire _logger.py and test_coroutine_sink.y files marked as changed in the PR diff, instead of just the few lines modified: #294 (files) ? 😕

Herm. That is p weird. I do not know! I have an .editorconfig in the dir above where I was working, so maybe it is that? (tho that has never happend to me before so I do not know)

@jessekrubin
Copy link
Contributor Author

@Delgan I think i figured it out. it a combo of me working on windows, having a .gitattributes and a .editorconfig above where I was working. I can revert and try again.

@Delgan
Copy link
Owner

Delgan commented Jul 15, 2020

Yeah, don't worry, I will make some tests next week-end and we'll figure out something. 👍

@jessekrubin
Copy link
Contributor Author

@Delgan I ACTUALLY FIGURED IT OUT! If you are curious. (it caused the same thing this afternoon in repo for my work)

I was working in a dir in a repo (where I cloned loguru). The repo above me had core.filemode set to true, so git was tracking the permissions which when changed cause git to think the whole file has changed.

@Delgan
Copy link
Owner

Delgan commented Jul 16, 2020

Thanks @jessekrubin for the investigation! 😄

Indeed, if we look at the raw commit this is mentioned: new file mode 100755.
Github doesn't seem to display this information.

@Delgan Delgan merged commit 0da3a28 into Delgan:master Jul 17, 2020
@Delgan
Copy link
Owner

Delgan commented Jul 17, 2020

It was actually a basic problem of line ending. Now it's merged, thanks for the work! :D

jhamon pushed a commit to pinecone-io/pinecone-python-client that referenced this pull request Oct 11, 2023
Bumps [loguru](https://github.com/Delgan/loguru) from 0.5.0 to 0.5.3.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/Delgan/loguru/releases">loguru's
releases</a>.</em></p>
<blockquote>
<h2>0.5.3</h2>
<ul>
<li>Fix child process possibly hanging at exit while combining
<code>enqueue=True</code> with third party library like
<code>uwsgi</code> (<a
href="https://redirect.github.com/Delgan/loguru/issues/309">#309</a>,
thanks <a
href="https://github.com/dstlmrk"><code>@​dstlmrk</code></a>).</li>
<li>Fix possible exception during formatting of non-string messages (<a
href="https://redirect.github.com/Delgan/loguru/issues/331">#331</a>).</li>
</ul>
<h2>0.5.2</h2>
<ul>
<li>Fix <code>AttributeError</code> within handlers using
<code>serialize=True</code> when calling <code>logger.exception()</code>
outside of the context of an exception (<a
href="https://redirect.github.com/Delgan/loguru/issues/296">#296</a>).</li>
<li>Fix error while logging an exception containing a non-picklable
<code>value</code> to a handler with <code>enqueue=True</code> (<a
href="https://redirect.github.com/Delgan/loguru/issues/298">#298</a>).</li>
<li>Add support for async callable classes (with <code>__call__</code>
method) used as sinks (<a
href="https://redirect.github.com/Delgan/loguru/issues/294">#294</a>,
thanks <a
href="https://github.com/jessekrubin"><code>@​jessekrubin</code></a>).</li>
</ul>
<h2>0.5.1</h2>
<ul>
<li>Modify the way the <code>extra</code> dict is used by
<code>LogRecord</code> in order to prevent possible
<code>KeyError</code> with standard <code>logging</code> handlers (<a
href="https://redirect.github.com/Delgan/loguru/issues/271">#271</a>).</li>
<li>Add a new <code>default</code> optional argument to
<code>logger.catch()</code>, it should be the returned value by the
decorated function in case an error occurred (<a
href="https://redirect.github.com/Delgan/loguru/issues/272">#272</a>).</li>
<li>Fix <code>ValueError</code> when using <code>serialize=True</code>
in combination with <code>logger.catch()</code> or
<code>logger.opt(record=True)</code> due to circular reference of the
<code>record</code> dict (<a
href="https://redirect.github.com/Delgan/loguru/issues/286">#286</a>).</li>
</ul>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/Delgan/loguru/blob/master/CHANGELOG.rst">loguru's
changelog</a>.</em></p>
<blockquote>
<h1><code>0.5.3</code>_ (2020-09-20)</h1>
<ul>
<li>Fix child process possibly hanging at exit while combining
<code>enqueue=True</code> with third party library like
<code>uwsgi</code>
(<code>[#309](Delgan/loguru#309)
&lt;https://github.com/Delgan/loguru/issues/309&gt;</code><em>, thanks
<code>@dstlmrk &lt;https://github.com/dstlmrk&gt;</code></em>).</li>
<li>Fix possible exception during formatting of non-string messages
(<code>[#331](Delgan/loguru#331)
&lt;https://github.com/Delgan/loguru/issues/331&gt;</code>_).</li>
</ul>
<h1><code>0.5.2</code>_ (2020-09-06)</h1>
<ul>
<li>Fix <code>AttributeError</code> within handlers using
<code>serialize=True</code> when calling <code>logger.exception()</code>
outside of the context of an exception
(<code>[#296](Delgan/loguru#296)
&lt;https://github.com/Delgan/loguru/issues/296&gt;</code>_).</li>
<li>Fix error while logging an exception containing a non-picklable
<code>value</code> to a handler with <code>enqueue=True</code>
(<code>[#298](Delgan/loguru#298)
&lt;https://github.com/Delgan/loguru/issues/298&gt;</code>_).</li>
<li>Add support for async callable classes (with <code>__call__</code>
method) used as sinks
(<code>[#294](Delgan/loguru#294)
&lt;https://github.com/Delgan/loguru/pull/294&gt;</code><em>, thanks
<code>@jessekrubin
&lt;https://github.com/jessekrubin&gt;</code></em>).</li>
</ul>
<h1><code>0.5.1</code>_ (2020-06-12)</h1>
<ul>
<li>Modify the way the <code>extra</code> dict is used by
<code>LogRecord</code> in order to prevent possible
<code>KeyError</code> with standard <code>logging</code> handlers
(<code>[#271](Delgan/loguru#271)
&lt;https://github.com/Delgan/loguru/issues/271&gt;</code>_).</li>
<li>Add a new <code>default</code> optional argument to
<code>logger.catch()</code>, it should be the returned value by the
decorated function in case an error occurred
(<code>[#272](Delgan/loguru#272)
&lt;https://github.com/Delgan/loguru/issues/272&gt;</code>_).</li>
<li>Fix <code>ValueError</code> when using <code>serialize=True</code>
in combination with <code>logger.catch()</code> or
<code>logger.opt(record=True)</code> due to circular reference of the
<code>record</code> dict
(<code>[#286](Delgan/loguru#286)
&lt;https://github.com/Delgan/loguru/issues/286&gt;</code>_).</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/Delgan/loguru/commit/f31e97142adc1156693a26ecaf47208d3765a6e3"><code>f31e971</code></a>
Bump version to 0.5.3</li>
<li><a
href="https://github.com/Delgan/loguru/commit/3394fdbd04efa7f5ec9c9625ec4ec34ca72b16e7"><code>3394fdb</code></a>
Update Changelog to reference <a
href="https://redirect.github.com/Delgan/loguru/issues/309">#309</a>
fix</li>
<li><a
href="https://github.com/Delgan/loguru/commit/44f677172ccf10ec60cbc46e4ac77394429211a7"><code>44f6771</code></a>
Fix removing simple queue in child processes (<a
href="https://redirect.github.com/Delgan/loguru/issues/325">#325</a>)</li>
<li><a
href="https://github.com/Delgan/loguru/commit/296635c33cbc735c3367728f24f07545dbfe19c5"><code>296635c</code></a>
Fix possible exception while formatting non-string message (<a
href="https://redirect.github.com/Delgan/loguru/issues/331">#331</a>)</li>
<li><a
href="https://github.com/Delgan/loguru/commit/b77f4fd23ac380fa2521dd2039c961af1218d1d1"><code>b77f4fd</code></a>
Correct simple typo in docs, propogates -&gt; propagates (<a
href="https://redirect.github.com/Delgan/loguru/issues/323">#323</a>)</li>
<li><a
href="https://github.com/Delgan/loguru/commit/e48f3e49228ada2f6495dd7eedc4ecd03530a1f6"><code>e48f3e4</code></a>
Bump version to 0.5.2</li>
<li><a
href="https://github.com/Delgan/loguru/commit/d93c19f6c1c53cc14e9afcb28b770f8aa43c28cd"><code>d93c19f</code></a>
Flesh out the few remaining incomplete annotations (<a
href="https://redirect.github.com/Delgan/loguru/issues/316">#316</a>)</li>
<li><a
href="https://github.com/Delgan/loguru/commit/084dffd16211e8494419ed5cf9c6859de8e76123"><code>084dffd</code></a>
Fix tests fails (since pytest upgrade) due to standard logging
misuse</li>
<li><a
href="https://github.com/Delgan/loguru/commit/9a40e58c74e786149b6a4c6097e24d50e0b40459"><code>9a40e58</code></a>
Fix error with &quot;enqueue=True&quot; and non-picklable exception (<a
href="https://redirect.github.com/Delgan/loguru/issues/298">#298</a>)</li>
<li><a
href="https://github.com/Delgan/loguru/commit/6f86f487590fe8f264a3bb26c722bf4be354687b"><code>6f86f48</code></a>
Refactor usage of &quot;inspect&quot; functions for simplification</li>
<li>Additional commits viewable in <a
href="https://github.com/Delgan/loguru/compare/0.5.0...0.5.3">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=loguru&package-manager=pip&previous-version=0.5.0&new-version=0.5.3)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the
[Security Alerts
page](https://github.com/pinecone-io/pinecone-python-client/network/alerts).

</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

2 participants