Skip to content

Conversation

@tedinski
Copy link
Contributor

Description of changes:

There was a bug where it was using arg[3] but in one case should have used arg[4]. Fixed, factored out the parsing step to its own function, then wrote tests for it, since clearly we can get this wrong. :)

Testing:

  • How is this change tested? new unit tests! woo

  • Is this a refactor change?

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@tedinski tedinski requested a review from a team as a code owner December 13, 2022 00:19
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Thanks for the bug fix.

On the long run, do you think it is still worth to manually parse the args instead of using a more stable library?

@tedinski
Copy link
Contributor Author

On the long run, do you think it is still worth to manually parse the args instead of using a more stable library?

I think the threshold is 'if we have to care about more than 1 argument'. I think we still benefit a lot from having very few deps in this proxy binary.

@tedinski
Copy link
Contributor Author

For reference,

clap = { version = "4.0.29", default-features = false, features = ["std"] }

gives

$ cargo build
   Compiling os_str_bytes v6.4.1
   Compiling bitflags v1.3.2
   Compiling clap_lex v0.3.0
   Compiling clap v4.0.29

as the only deps and adds 3.5 seconds to build time on my laptop. So yeah, as soon as there's any more complexity to this, probably switch to (non-derive-based) clap is not super high-cost.

@tedinski tedinski merged commit ba389c6 into model-checking:main Dec 13, 2022
@tedinski tedinski deleted the arg-parse-proxy branch December 13, 2022 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants