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

Make cleaner separation between parser and main. #666

Closed
wants to merge 0 commits into from

Conversation

jonathf
Copy link
Collaborator

@jonathf jonathf commented Jan 29, 2022

Currently the main interface creates CLArgs to parser arguments, which (depending on input) get passed down into the vroom machinery.
For the most part this is done cleanly in such a way that you don't need CLArgs to use any machinery, with input parser being the only exception.

I propose we make the interface cleaner by keeping CLArgs contain to the main file and just passing it's attribute along explicitly.

Issue

#665

Tasks

  • Update CHANGELOG.md
  • review

@jonathf jonathf force-pushed the refactor/parser_sign branch from 4f3cf65 to 943e61d Compare January 29, 2022 22:13
@jcoupey jcoupey added this to the v1.12.0 milestone Jan 31, 2022
Copy link
Collaborator

@jcoupey jcoupey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple change but a good readability improvement. I guess this is the benefit of having people look at the code from a "downstream" perspective! ;-)

src/utils/input_parser.h Outdated Show resolved Hide resolved
src/utils/input_parser.h Outdated Show resolved Hide resolved
@jonathf
Copy link
Collaborator Author

jonathf commented Jan 31, 2022

I've pushed updated changes now.

To avoid the issue with requiring known amount_size, I set some asserts in place to assure that amount_size is always defined first.

If you think this is too invasive, I will be happy to roll back to the first iteration (with suggestions) and continuing the conversation for a new PR in the #665.

Copy link
Collaborator

@jcoupey jcoupey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clean. We're loosing constness of Input::_amount_size but I think it's fine since it's private and users have to do things right anyway via set_amount_size.

src/structures/vroom/input/input.cpp Outdated Show resolved Hide resolved
src/structures/vroom/input/input.h Outdated Show resolved Hide resolved
src/utils/input_parser.cpp Outdated Show resolved Hide resolved
@jonathf jonathf force-pushed the refactor/parser_sign branch from 20be5e5 to d1c6250 Compare January 31, 2022 16:48
@jonathf
Copy link
Collaborator Author

jonathf commented Jan 31, 2022

I've applied alll suggestions and rebased the branch to the tip of master.

@jcoupey jcoupey closed this Feb 1, 2022
@jcoupey jcoupey force-pushed the refactor/parser_sign branch from c605ba1 to 467c3db Compare February 1, 2022 13:11
@jcoupey
Copy link
Collaborator

jcoupey commented Feb 1, 2022

The PR shows up as closed since I messed a bit with the branch but it's merged all right. Looks like GitHub is confused as I squashed the last bugfixes commits into a single one and pushed the merge commit to master before force-pushing this branch.

@jcoupey jcoupey deleted the refactor/parser_sign branch February 1, 2022 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants