3 ms·

Wow, that's a lot of code smell to me. Why specify all the specific fragments of mypy.nodes, instead of import *? Docstrings in the middle of code should just

Wow, that's a lot of code smell to me. Why specify all the specific fragments of mypy.nodes, instead of import *? Docstrings in the middle of code should just die 1/2 of the screen is wasted Why use a case statement if it only has one match? I can't even figure out what lines 43-55 do, but that's a lot of nesting

serpent_skis 4y ago
The reason for using a match statement, even though there is only “one” case, is because it’s matching against a complex and (as you mentioned) deeply nested pattern. Lines 43-56 match the current AST node against the pattern (shown here as pseudo-python): with open(_) as f: _ = f.read() (Where _ can be anything, and f can be any name). The traditional alternative to lines 43-56 would be dozens of lines that check the top level of the AST, and if that matches against what the top level of the pattern is expecting, diving one layer deeper and checking that level against the next level of the pattern, etc, until you reach the leaves of your pattern. Once your familiar with pattern matching syntax, this is much easier to quickly read/grok/audit.
- UncleEntity 4y ago
  So it can do multi-level tree matching? Very interesting… One of the projects I occasionally poke at is a burs (bottom-up rewrite system) generator used to do cost-based tree rewriting. Think this is a concept that might make sense in that context, just need to find the time to play…
david2ndaccount 4y ago
> Why specify all the specific fragments of mypy.nodes, instead of import *? glob imports are discouraged as it is hard to figure out where a name comes from (or if it’s just a typo). > Docstrings in the middle of code should just die 1/2 of the screen is wasted You mean the class level doc string? That shows up in the help of that class, so there is nowhere else to put it. I also don’t see the point of a match with a single case.
hexane360 4y ago
Does "code smell" just mean "style I don't like"? Most of your concerns have nothing to do with what the code is actually doing. Line 43-55 is the scrutinee (pattern) of the case statement. It only binds values to the given variables and enters the block if `node` matches the quite complex structure shown there. The alternative would be doing something repetitive like this: if not isinstance(node, WithStmt): return expr = node.expr if not len(expr) == 1: return if not isinstance(expr, CallExpr): return callee = expr.callee if not (isinstance(callee, NameExpr) and callee.name == 'open'): return ... etc ... You could do the same thing with duck typing and try/except, but the semantics would be different (sometimes you DO want nominal typing, even in python).
- mikewarot 4y ago
  Why not just something similar to (forgive the pascal) if isinstance(node, WithStmt) AND (len(node.expr) = 1) AND isinstance(node.expr, CallExpr) AND isinstance(node.expr.callee,NameExper) AND (node.expr.callee.name == 'open') then begin do stuff here end;
deleted 4y ago
[deleted]
ttymck 4y ago
> I can't even figure out what lines 43-55 do, but that's a lot of nesting It's a very elegant pattern match on a deeply nested structure. It's declarative, not imperative, so if you are concerned about "nesting complexity", you need not be. How would you implement this otherwise?
- mikewarot 4y ago
  I honestly don't understand what's going on... but in Pascal I'd use short circuit expression evaluation to make the comparisons as I go into the depths. There's too many layers of nesting, in too foreign a syntax for me to even parse. I suspect it could be done a clearer manner in pascal.
  - ttymck 4y ago
    I would truly love to see it in pascal. I would be happy to give my opinion on it as a "foreign syntax" to me
dosisod 4y ago
In addition to some of the other answers about the match statement, mCoding has this excellent video[0] explaining the match statement, specifically using it to match complex AST trees, like Refurb does. Note, not all of these checks use the match statement[1], sometimes it is more verbose to use a match statement vs a normal if/else statement. [0]: https://youtu.be/ASRqxDGutpA?t=470 https://youtu.be/ASRqxDGutpA?t=470 [1]: https://github.com/dosisod/refurb/blob/master/refurb/checks/logical/use_or.py https://github.com/dosisod/refurb/blob/master/refurb/checks/...

serpent_skis 4y ago

The reason for using a match statement, even though there is only “one” case, is because it’s matching against a complex and (as you mentioned) deeply nested pattern. Lines 43-56 match the current AST node against the pattern (shown here as pseudo-python): with open(_) as f: _ = f.read() (Where _ can be anything, and f can be any name). The traditional alternative to lines 43-56 would be dozens of lines that check the top level of the AST, and if that matches against what the top level of the pattern is expecting, diving one layer deeper and checking that level against the next level of the pattern, etc, until you reach the leaves of your pattern. Once your familiar with pattern matching syntax, this is much easier to quickly read/grok/audit.

UncleEntity 4y ago
So it can do multi-level tree matching? Very interesting… One of the projects I occasionally poke at is a burs (bottom-up rewrite system) generator used to do cost-based tree rewriting. Think this is a concept that might make sense in that context, just need to find the time to play…

david2ndaccount 4y ago

> Why specify all the specific fragments of mypy.nodes, instead of import *? glob imports are discouraged as it is hard to figure out where a name comes from (or if it’s just a typo). > Docstrings in the middle of code should just die 1/2 of the screen is wasted You mean the class level doc string? That shows up in the help of that class, so there is nowhere else to put it. I also don’t see the point of a match with a single case.

hexane360 4y ago

Does "code smell" just mean "style I don't like"? Most of your concerns have nothing to do with what the code is actually doing. Line 43-55 is the scrutinee (pattern) of the case statement. It only binds values to the given variables and enters the block if `node` matches the quite complex structure shown there. The alternative would be doing something repetitive like this: if not isinstance(node, WithStmt): return expr = node.expr if not len(expr) == 1: return if not isinstance(expr, CallExpr): return callee = expr.callee if not (isinstance(callee, NameExpr) and callee.name == 'open'): return ... etc ... You could do the same thing with duck typing and try/except, but the semantics would be different (sometimes you DO want nominal typing, even in python).

mikewarot 4y ago
Why not just something similar to (forgive the pascal) if isinstance(node, WithStmt) AND (len(node.expr) = 1) AND isinstance(node.expr, CallExpr) AND isinstance(node.expr.callee,NameExper) AND (node.expr.callee.name == 'open') then begin do stuff here end;

deleted 4y ago

[deleted]

ttymck 4y ago

> I can't even figure out what lines 43-55 do, but that's a lot of nesting It's a very elegant pattern match on a deeply nested structure. It's declarative, not imperative, so if you are concerned about "nesting complexity", you need not be. How would you implement this otherwise?

mikewarot 4y ago
I honestly don't understand what's going on... but in Pascal I'd use short circuit expression evaluation to make the comparisons as I go into the depths. There's too many layers of nesting, in too foreign a syntax for me to even parse. I suspect it could be done a clearer manner in pascal.
- ttymck 4y ago
  I would truly love to see it in pascal. I would be happy to give my opinion on it as a "foreign syntax" to me

dosisod 4y ago

In addition to some of the other answers about the match statement, mCoding has this excellent video[0] explaining the match statement, specifically using it to match complex AST trees, like Refurb does. Note, not all of these checks use the match statement[1], sometimes it is more verbose to use a match statement vs a normal if/else statement. [0]: https://youtu.be/ASRqxDGutpA?t=470 https://youtu.be/ASRqxDGutpA?t=470 [1]: https://github.com/dosisod/refurb/blob/master/refurb/checks/logical/use_or.py https://github.com/dosisod/refurb/blob/master/refurb/checks/...