core: Add B(bugbear) ruff rules #25520
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
dosubot
bot
added
size:L
| if hasattr(res, "target_run_id") | ||
| else str(run.id) | ||
| ) | ||
| run_id = str(getattr(res, "target_run_id", run.id)) |
There was a problem hiding this comment.
What if res.target_run_id is None ?
cbornet
force-pushed
the
ruff-core-b
branch
2 times, most recently
from
5299dff
to
a26424a
Compare
| @@ -516,15 +516,19 @@ def __copy__(self) -> _TracerCore: | |||
|
|
|||
| def _end_trace(self, run: Run) -> Union[None, Coroutine[Any, Any, None]]: | |||
| """End a trace for a run.""" | |||
| return None | |||
There was a problem hiding this comment.
due to PyCQA/flake8-bugbear#301
Do we prefer to ignore the rule with noqa ?
cbornet
force-pushed
the
ruff-core-b
branch
7 times, most recently
from
bcddb7c
to
1bbcf0b
Compare
| @@ -568,7 +569,7 @@ def similarity_search_with_relevance_scores( | |||
| if similarity >= score_threshold | |||
| ] | |||
| if len(docs_and_similarities) == 0: | |||
| warnings.warn( | |||
| logger.warning( | |||
There was a problem hiding this comment.
From https://docs.python.org/3/howto/logging.html#when-to-use-logging :
warnings.warn() in library code if the issue is avoidable and the client application should be modified to eliminate the warning
A logger’s warning() method if there is nothing the client application can do about the situation, but the event should still be noted
cbornet
changed the title
dosubot
bot
added
the
lgtm
| @@ -748,7 +748,7 @@ def is_async_generator( | |||
| """ | |||
| return ( | |||
| inspect.isasyncgenfunction(func) | |||
| or hasattr(func, "__call__") | |||
There was a problem hiding this comment.
I think this was on purpose cc @hinthornw I think
There was a problem hiding this comment.
It comes from https://docs.astral.sh/ruff/rules/unreliable-callable-check/
Is it a false positive ? Should the rule be ignored here ?
There was a problem hiding this comment.
Here this is explicitly just a guard so that we don't err when checking inspect.isasyncgenfunction(func.__call__) in the next line, so i think it's a false positive.
There was a problem hiding this comment.
could alternatiely do inspect.isasyncgenfunction(getattr(func, '__call__', None)) if we want the linter to be happy
|
|
||
| def _on_retriever_error(self, run: Run) -> Union[None, Coroutine[Any, Any, None]]: | ||
| """Process the Retriever Run upon error.""" | ||
| return None |
There was a problem hiding this comment.
this doesn't make a ton of sense, its either an abstract method or it isn't
There was a problem hiding this comment.
seems like separate issue from this pr (having nothing is equivalent to returning None)
There was a problem hiding this comment.
See previous comment: https://github.com/langchain-ai/langchain/pull/25520/files/dd1df352ffb2731807f10224b0919039782939d7#r1720771953
It's not an abstract method, but ruff thinks it should be if we don't add return None.
I can add noqa instead if you prefer.
efriis
changed the title
No description provided.