YousufFFFF commented on PR #37673: URL: https://github.com/apache/beam/pull/37673#issuecomment-3961481530
> I agree with all of Mohamed's comments so far. > > In particular, the one about simply just rewriting this to use bespoke code instead of the package. > > As a first pass, add a few simple test cases that check the returned output. This is just good practice when doing a refactoring to make sure that some things are working at least as good as they were before. This is not a high bar, at least, due to not having any tests in the first place! Even just a 3-4 very basic pipelines as a smoke test would go a long way. > > This dot runner is very old, so it is unfortunate that it was authored without tests, since it was actively being used to look at pipeline shapes. > > Also, do take a look at the python "render" runner for inspiration, which also creates dot representations. > > https://github.com/apache/beam/blob/9ee4fe64f2b99cb9b555ba65be202dcde40ebe74/sdks/python/apache_beam/runners/render.py#L135 > > (Admittedly, having done a different python -> go conversion for prism's Fusion handling, the python handling is very Set Theory based rather than graph based, so it can take a moment to grasp what it's doing.) Thanks @lostluck - I’ll take a look at the Python render runner for reference. That’s a good suggestion, especially since it already handles DOT generation in a more structured way. I’ll review its approach and see if there are any patterns or simplifications we can adopt here while keeping the Go implementation idiomatic. Appreciate the pointer 👍 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
