refactor: rename exe_trace to concrete_playback and det_vals to concrete_vals#1548
Conversation
celinval
left a comment
There was a problem hiding this comment.
Thanks for doing this. Very thorough!
I really wish we could make a few changes to the way we take arguments. See my comment below but I'm afraid the change might require us to move out of structopt. Not sure.
I would prefer using print instead of JustPrint to make it less verbose. None of those are blockers, so I'm approving the PR.
Thanks
| /// Generate executable trace test case and print it to stdout. | ||
| /// Generate concrete playback unit test and either 1) just print it to stdout or 2) add it in-place to the source code. | ||
| /// This option does not work with `--output-format old`. | ||
| #[structopt( |
There was a problem hiding this comment.
I couldn't figure out good ways with structopt to do the following:
- Use kebab-case for the arguments.
- Allow users to use --concrete-value without an argument (and default it to
print).
I tried clap-rs/clap#2945 (reply in thread) to rename the arguments and clap-rs/clap#892 to make the argument optional but couldn't get them to work. Maybe they only work with clap directly.
@tedinski any suggestions?
There was a problem hiding this comment.
I don't know how to use kebab-case for the arguments.
There was a problem hiding this comment.
I considered allowing users to use --concrete-value without an argument (and default to print). I spent about 30 mins looking through this, and I couldn't find any clean way to do it. Here's what I found:
- I could do it if I made
concrete_playback: Option<Option<ConcretePlaybackMode>>. However, that has a really bad effect on code quality IMO. No concrete playback would beNone, concrete playback in print mode would beSome(None), and concrete playback in in-place mode would beSome(Some(InPlace)). This doesn't match my intuition as a programmer. - Even if I set the default value for
ConcretePlaybackModeto bePrint,structoptdoesn't seem to be picking this up. When I leave just--concrete-playback, it errors out and says that I need to fill in something. I'm not allowed to add adefault_valueoption to thestructoptmacro because the type is anOption. - structopt issue #123 asks for the exact same thing. They closed it with a PR, but I can't seem to figure out how the PR actually fixes the issue.
- To create a custom parser that functions properly, it would need to take both the number of occurrences and the actual strings. There isn't support for such a parser yet.
Because I didn't find any clean way to implement this, I decided to force the user to specify either Print or InPlace. This isn't ideal, but it does have the plus-side of making the operating mode explicit.
Description of changes:
See title. Note: there's still probably things that I forgot to rename, but I tried my best to catch as much as I could in this pass.
Resolved issues:
Resolves #1505, renaming.
Testing:
This change is tested by the existing tests in
tests/ui/concrete-playback. I manually re-ran those tests to make sure they played back correctly.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.