LineaLabs / lineapy

Move fast from data science prototype to pipeline. Capture, analyze, and transform messy notebooks into data pipelines with just two lines of code.
https://lineapy.org
Apache License 2.0
664 stars 58 forks source link

LIN-560 separate nodetransformers #840

Closed lionsardesai closed 1 year ago

lionsardesai commented 1 year ago

Description

This PR moves the code to handle changes in ast over different python versions to their own transforms. this way nodetransformer remains agnostic of the version and makes future additions plug-n-playable.

Fixes LIN-560

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

existing test + some modified unit tests

andycui97 commented 1 year ago

Can you leave a description for the PR?

andycui97 commented 1 year ago

Can we also put the logic in this commit into a NodeTransformer? https://github.com/LineaLabs/lineapy/commit/8440fda96277d4efe619c940df9a284a4ac3e37e

Call it something like the "Multiline Expression Transformer"

lionsardesai commented 1 year ago

Can we also put the logic in this commit into a NodeTransformer? 8440fda

Call it something like the "Multiline Expression Transformer"

I see this edit as slightly different from others. All the others are/will either 1) swapping out ast nodes for other ast or ast-replacement nodes 2) swapping out ast nodes for custom nodes (lineanodes - future) 3) re-doing the node order (conditional- future)

This operation only touches the source location and does not touch the nodes themselves so i feel it should not be pulled out. However, if we see merit in this action in the future, hopefully this new structure makes it very easy to implement.

andycui97 commented 1 year ago

Can we also put the logic in this commit into a NodeTransformer? 8440fda Call it something like the "Multiline Expression Transformer"

I see this edit as slightly different from others. All the others are/will either

  1. swapping out ast nodes for other ast or ast-replacement nodes
  2. swapping out ast nodes for custom nodes (lineanodes - future)
  3. re-doing the node order (conditional- future)

This operation only touches the source location and does not touch the nodes themselves so i feel it should not be pulled out. However, if we see merit in this action in the future, hopefully this new structure makes it very easy to implement.

Are you not touching an AST Node when you do something like value_node.lineno = min(node.lineno, value_node.lineno)?

lionsardesai commented 1 year ago

Can we also put the logic in this commit into a NodeTransformer? 8440fda Call it something like the "Multiline Expression Transformer"

I see this edit as slightly different from others. All the others are/will either

  1. swapping out ast nodes for other ast or ast-replacement nodes
  2. swapping out ast nodes for custom nodes (lineanodes - future)
  3. re-doing the node order (conditional- future)

This operation only touches the source location and does not touch the nodes themselves so i feel it should not be pulled out. However, if we see merit in this action in the future, hopefully this new structure makes it very easy to implement.

Are you not touching an AST Node when you do something like value_node.lineno = min(node.lineno, value_node.lineno)?

Punted to LIN-708