-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add missing-timeout warning for requests.Session
object
#9526
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @joel6948, do you mind expanding missing-timeout
instead of creating a new extension please ?
sure |
#9527 (comment) |
requests.Session
object
@@ -71,13 +71,11 @@ class MethodArgsChecker(BaseChecker): | |||
) | |||
def visit_call(self, node: nodes.Call) -> None: | |||
self._check_missing_timeout(node) | |||
self._check_positional_only_arguments_expected(node) | |||
# Other checks... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Other checks... | |
self._check_positional_only_arguments_expected(node) |
This should stay, you can delete pylint/extensions/requests_timeout_plugin.py and add your use case in _check_missing_timeout
directly (without adding a new message, just use the existing missing-timeout
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you added a python file in the documentation, but you need to modify the code / add your logic here instead:
pylint/pylint/checkers/method_args.py
Lines 76 to 100 in 27b1ae7
def _check_missing_timeout(self, node: nodes.Call) -> None: | |
"""Check if the call needs a timeout parameter based on package.func_name | |
configured in config.timeout_methods. | |
Package uses inferred node in order to know the package imported. | |
""" | |
inferred = utils.safe_infer(node.func) | |
call_site = arguments.CallSite.from_call(node) | |
if ( | |
inferred | |
and not call_site.has_invalid_keywords() | |
and isinstance( | |
inferred, (nodes.FunctionDef, nodes.ClassDef, bases.UnboundMethod) | |
) | |
and inferred.qname() in self.linter.config.timeout_methods | |
): | |
keyword_arguments = [keyword.arg for keyword in node.keywords] | |
keyword_arguments.extend(call_site.keyword_arguments) | |
if "timeout" not in keyword_arguments: | |
self.add_message( | |
"missing-timeout", | |
node=node, | |
args=(node.func.as_string(),), | |
confidence=INFERENCE, | |
) |
Please note that we're using inference which can handle case where a variable is used and not request.Session
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but in the main branch is it showing inference which can handle case where a variable is used and not request.Session directly. or is it my mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure. I think we handle a call to request.get (and other see self.linter.config.timeout_methods
) (directly or inferred) but not Session. It should be able to do both (?). Maybe the solution is as simple as changing the default for timeout_methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did the change a bit can you is it working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to test this would be to create a functional tests, see here: https://pylint.readthedocs.io/en/stable/development_guide/contributor_guide/tests/writing_test.html#functional-tests and here https://github.com/pylint-dev/pylint/blob/main/tests/functional/m/missing/missing_timeout.py
You're also going to need to add a changelog entry, see:
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 9f99786 |
@joel6948 Would you like to keep working on this? |
This PR needs take over because because it has been open 8 weeks with no activity. |
some issue fixes
#9517 for this issue