Implementing TRY400

[Flowake]

Currently implementing TRY400 (#2056)

I cannot find the documentation for the ast parser used by ruff, where could I find it ? That would make my life easier.

[charliermarsh]

We use the RustPython AST, defined here: https://github.com/RustPython/RustPython/blob/main/compiler/ast/src/ast_gen.rs

It's actually generated by reading from the Python ASDL files, so it's pretty similar to the CPython AST.

[Flowake]

I have translated the raw python logic from the tryceratops library and I copied other rules to handle the reporting.

I think the violation might be triggered for any method "error" used in the except block. I would need to ensure that the object that the method is called on is a logger, but I'm not sure about how to do that.

[charliermarsh]

Yeah, we don't have any way to know for sure that the object is a logger. Like the original plugin, this will raise on any .error(...) call. The best we can do, probably, is trade false negatives for false positives by flagging specific cases, like logging.error(...) or logger.error(...) (that is, literally match on the names logging and logger).

[charliermarsh]

(Or we just accept this limitation, given that it exists "upstream".)

[Flowake]

I think sticking to the "upstream" plugin when there are no clear better alternatives for the implementation is preferable.

If you're ok with the code I can make this PR ready for review.

[Flowake]

I fixed the last issue, it's ready for review !

[charliermarsh]

Sounds good. I'll give this a look today and merge.

[Flowake]

I was reading about other rules and their auto fixing part and was wondering what is the approach in this case: we know that the rule will have false negatives, which an autofix would definitely break.

If we want to auto fix this rule, we might actually switch to checking specific cases (like logging and logger) to ensure we only have false positives and then autofix it by changing .error to .exception. But if there was a custom object called logger with an error method and no exception method, it would also produce broken code.

[charliermarsh]

I think this one is too uncertain to allow an autofix. The one case we could fix is, if the call is import logging; logging.error, then we have the ability to verify that call path and fix it. But any other case is likely too dangerous.

[charliermarsh]

Quick tip for the future: in order to get the fixture to run on CI etc., we need to add a test case here via this macro.

[charliermarsh]

I decided to make this a bit stricter. I'm just erring on the side of false negatives these days a little bit, especially for checks like this that lack strong static verifiability. Sorry for the last minute change!