Skip to content

Conversation

@nolan-veed
Copy link
Contributor

Why

For #656

What

  • Created Configs to hold all the configs.
  • Wrote functions to setup various configs.
  • Wrote functions to setup input and sender.

Testing

WIP

@github-actions github-actions bot added the S-work-in-progress status: PR is still in progress and changing label Mar 25, 2024
@nolan-veed
Copy link
Contributor Author

@gavv Can I get some thoughts on approach?

@github-actions github-actions bot added the S-needs-rebase status: PR has conflicts and should be rebased label Apr 9, 2024
@github-actions
Copy link

github-actions bot commented Apr 9, 2024

🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts.

@gavv
Copy link
Member

gavv commented Apr 9, 2024

Thanks, will take a look in upcoming days!

I think the conflict is from this commit from here.

@gavv
Copy link
Member

gavv commented Apr 9, 2024

Also: d873794

@gavv gavv added S-ready-for-review status: PR can be reviewed contrib PR not by a maintainer labels Apr 9, 2024
@gavv
Copy link
Member

gavv commented May 30, 2024

Hi, sorry for delay. I like the approach!

A few minor comments / thoughts:

  • Instead of a big Configs struct with all the things it maybe would be better to pass every argument explicitly.

    This way it would be clear what variables are used by what functions, and so easier to understand the flow.

  • All functions are named setup_xxx(), but actually some of them do different things.

    Some build configs, some open devices, some configure endpoints. I suggest to reflect that in their names, again, it should make it easier to understand the flow in main().

  • It would be nice if we could create all top-level objects in main().

    It seems currently the only exception is input_source, which we create in separate function. We could keep endpoint parsing in a function, but move construction of input source to main().

  • The piece where we propagate parameters from io_config to sender_config can be extracted into a small function too.

    This way this step will be reflected in main() more explicitly and main() will be less cluttered.

@gavv
Copy link
Member

gavv commented May 30, 2024

If you'll decide to proceed with this PR, I think it's good idea to review & merge just one tool (e.g. roc_send) and then work on the rest, because we can find more pitfalls during review.

@gavv gavv added S-needs-revision status: Author should revise PR and address feedback and removed S-ready-for-review status: PR can be reviewed labels May 30, 2024
@gavv
Copy link
Member

gavv commented Sep 12, 2024

Hi, I've addressed this issue in 96ac193 (a big commit that also includes this refactoring).

Thanks for inspiration!

@gavv gavv closed this Sep 12, 2024
@gavv gavv added S-dismissed status: Decided not to implement and removed S-work-in-progress status: PR is still in progress and changing S-needs-revision status: Author should revise PR and address feedback S-needs-rebase status: PR has conflicts and should be rebased labels Sep 12, 2024
@gavv gavv added S-moved status: Transitioned to another issue or PR and removed S-dismissed status: Decided not to implement labels Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contrib PR not by a maintainer S-moved status: Transitioned to another issue or PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants