Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Splitting main() functionality #710

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

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 work in progress Pull request 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 needs rebase Pull request has conflicts and should be rebased label Apr 9, 2024
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 ready for review Pull request can be reviewed contribution A pull-request by someone else except maintainers 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 needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution A pull-request by someone else except maintainers needs rebase Pull request has conflicts and should be rebased needs revision Pull request should be revised by its author work in progress Pull request is still in progress and changing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants