-
Notifications
You must be signed in to change notification settings - Fork 3
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 --mutation-info-file optional #195
Conversation
Encapsulating whether we want to build the mutation tree in protobufs seemed like the best choice. We can't readily use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do a proper review yet, but at a glance I don't like the idea of adding a use
field to the top-level protobuf object. Presumably the idea is that if this field is false / 0, the protobuf is not in use.
I think it would be better to pass an optional around, such that if this optional is empty there is no protobuf object at all, and if it's not empty there is a complete protobuf object, so that no change to the protobuf description is required.
Would that be possible or is there a good reason why the use
approach is needed?
@JamesLee-Jones as part of this PR, can you update the project README so that the instructions no longer use this option (and also remove the comments that explain that the option is mandatory but doesn't matter for purposes of the README examples)? |
@JamesLee-Jones in my latest push I updated the README so that it does use the generated .json file in the math library example, so it's only for the simple pi example that we'd want to remove all trace of it. |
da06368
to
b931425
Compare
656c055
to
1be092d
Compare
I now have bandwidth to look at this again if you want to rebase it and revise it if needed. |
I'll revise it today along with the rest of the reviewed PRs. |
0bc0266
to
c214728
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good - small changes requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! A few final wording things - can you attend to them and then merge? I don't need to re-review.
@JamesLee-Jones This needs a rebase, but otherwise should be good to go if you're able to attend to the minor edits. |
Make `--mutation-info-file` parameter optional as writing this file can cause issues if the mutation tree is too deep.
71d86d7
to
854321c
Compare
Make
--mutation-info-file
parameter optional as writing this file can cause issues if the mutation tree is too deep.