-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-128742: Remove comments from find_assignment_target
result
#128743
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
base: main
Are you sure you want to change the base?
Conversation
def test_PyStackRef_FromPyObjectNew_with_comment(self): | ||
input = """ | ||
inst(OP, (-- value)) { | ||
// Comment is ok |
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.
Can we have more /* ... */
comments as well. And more than one comment as well? something like:
inst(OP, (-- value)) {
// Comment is ok
value = ...
}
inst(OP, (-- value)) {
/* comment is ok */
value = ...
}
inst(OP, (-- value)) {
/*
comment is ok
*/
value = ...
}
inst(OP, (-- value)) {
// comment
// comment
value = ...
}
inst(OP, (-- value)) {
// comment
value = ...
// comment
}
(I don't know which comments are actually accepted). The output being always the same, you can parametrize the test as follows:
output = ...
line = "value = PyStackRef_FromPyObjectNew(GETITEM(FRAME_CO_CONSTS, oparg));"
for src in [
("//", line),
("// comment", line),
("/* comment */", line),
...
]:
lines = textwrap.indent('\n'.join(src), " " * 4)
input = f"inst(OP, (-- value)) {{\n{lines}\n}}"
with self.subTest(src=src):
self.run_cases_test(input, output)
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'd rather we do all these tests explicitly. Example you provided is hard to read IMO.
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.
Considering the rest of the file seems to never parametrize the tests, maybe explicit is better. I personally find it quite explicit but if you think it reads worse, keep the current style!
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.
(However, we still need more tests. To ease debugging, let's create different test functions for each of them as well?)
find_assignment_target
result #128742