Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cli change for file based request source #563
Cli change for file based request source #563
Changes from 186 commits
99f45d1
075a4cf
ecbf407
8fb11b6
032dbba
953e953
6185edb
cded6ad
fbfe523
0d70179
cdb7efe
f15bb47
59359a2
f63df91
406bd6b
f0b697b
8425e48
fb4f372
c6eae6c
96ae38b
b17240f
5c5a146
29591ff
7d928a9
ff82d95
98381a4
83ac556
38c5084
fa4e285
8551c76
437de5e
6cef87e
c84ba4d
77d33f9
73aa686
ef77d65
b9182a0
e38dd80
21734e4
7cda9d2
f827aca
d194736
7820cf9
bee7805
3db68ce
c0ec4dd
a1dd3f2
0b25261
c3fa23c
547977e
c300203
8f4d4d9
5ae2d51
fe74b6d
6d87aea
ca7a0c2
967e3f8
14b7d79
c4985bb
922639a
81dfece
bf08661
34900c5
768fa81
e93bb58
f47cbd4
7c5c6b2
1b02cec
4d75602
c74f535
9949fb8
fcf10d3
fd9744d
68c1037
883ee30
8c34b25
6ce7e74
92927e5
a62f2d5
16f89c6
f4257e3
d075588
eb10a3b
cdbd6bf
17acc5f
55a0e62
fa23aad
b8e2447
248f439
91703b5
bbc0c2a
517c83d
c1d0428
bc86c09
33e3564
338a874
402ada2
017481f
3ed43fc
247b71b
718e4cd
50c0c07
171fb2e
5df83ed
0af3ffd
ff90e59
e57c05f
55244b0
3d0b1a2
c710d37
fbb2fdc
ae0f4a8
e092de0
1b87b71
6a72987
a080eec
19b7aa0
dae3098
a4b6fb1
55f685e
59c5b94
2ea0c5e
d40a073
7146079
7ab1132
72c23d0
2b1170c
9dc3c4b
b555a71
3924eee
75eca07
9c93ff9
0040b33
e2af886
edae9be
949d731
59d490f
078329a
63e5d2b
530cc9c
22cc3f8
32489d0
324bb23
5d6bb26
32ae13d
12708ac
eff2f4e
b890df4
a907913
49a47a4
b711028
8a2d792
22b93aa
7b9c175
bab49fc
7184695
fc7eda6
49e6eec
43de3d8
fef08dd
264355c
fa4d4c9
546449a
1a28a30
fa2adc5
01959ea
6696b5e
6168223
3043405
309f75b
4a63c18
55a7460
fdb84d2
592345a
e98211f
436f952
fee4328
c37cde2
8e56850
ac18ebb
66cd5ed
fc254f0
9a409dd
314f924
587cf3c
8401dd5
8ee66df
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Two nit asks on this set of TODOs:
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.
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 think you're overdescribing here. It's also particularly confusing because the code you're describing also doesn't appear to be the code that it immediately follows this comment.
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 found this comment helpful to me, so I thought it made sense to keep it. I think I would have found these lines odd if I didn't have them.
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.
Sorry -- my previous code comment wasn't particularly helpful/directive. I agree that comments here would be helpful, but I'm not finding this comment to clarify the code for me, so let me re-explain better:
In general, comments are most helpful when we explain the why, rather than the what. This comment is actually doing a little bit of both. One of the reasons why I think you're finding the need to explain "what" is that the comments aren't actually positionally with the code that they're explaining.
Rather than explaining a whole series of actions in one comment, can we split up the comments to be directly tied to their code sections?
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.
One thing that might make this easier to parse is if we use more descriptive names, rather than s1 and s2. Maybe expected_options_yaml and actual_options_yaml? Or whatever names you think would be accurate/helpful.
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.
For sure.
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.
Consider also adding the following comparison, as I suspect the yaml/text based comparison may miss edge cases.
EXPECT_TRUE(util(*(options_from_proto.toCommandLineOptions()), *command));
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.
Ok. Understood. That makes sense.
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.
@dubious90 Wanted to let you know that I was adding this back in since this reasoning makes sense to me.
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'm having a really hard time parsing this. There are two things that might help here:
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'm not sure that 1 works for me here. I might be missing something, but it seems like there are issues with the newlines. At least it seems like using R"( makes you keep the newlines if you space them into separate lines. I can make each line a separate raw string though, I'm not sure that this makes it more readable, but maybe you'll find this preferable?
2. I've kind of tried to do this. Hope that this approach looks ok.