RustPython / Parser

MIT License
72 stars 28 forks source link

Add `Ast` as top level enum #58

Closed youknowone closed 1 year ago

youknowone commented 1 year ago

One of the possible solution of #48

youknowone commented 1 year ago

cc @MichaReiser

MichaReiser commented 1 year ago

Thanks for working on this and sorry for my late reply. I've been busy with other work and haven't had time to think more about the problem.

I still think that I would favor the flat structure over the nested one because:

Would you mind if I change the enum to a flat structure? I will also introduce a AstRef as part of that work because that's actually the enum that I need first.

youknowone commented 1 year ago

Because current Ast enum is the counter part of python side ast.AST(each rust enum matches to inheritance from Python), adding a new enum for flat structure seems better if you need one. Before adding the Ast type, the top level ast.AST was the only missing type from Rust side. On the other hand, adding is_stmt_if() to current Ast seems also reasonable if it fits your requirements. (probably not if you need to use match)

youknowone commented 1 year ago

I wish conversion between Ast and other types like Stmt and Expr doesn't require much cost. If we can achieve it with flat structure, that's also fine.

MichaReiser commented 1 year ago

Because current Ast enum is the counter part of python side ast.AST(each rust enum matches to inheritance from Python), adding a new enum for flat structure seems better if you need one. Before adding the Ast type, the top level ast.AST was the only missing type from Rust side. On the other hand, adding is_stmt_if() to current Ast seems also reasonable if it fits your requirements. (probably not if you need to use match)

I'll add it on the Ruff side if you want to keep using the Ast node for CPython compatibility

youknowone commented 1 year ago

Yes, that will be a good option.