| Message ID | 20220803103941.313008-1-dceara@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] tests: Fix flake8 reported warning due to line length. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 8/3/22 12:39, Dumitru Ceara wrote: > Starting with flake8>=5.0.1 we got the following report: > tests/check_acl_log.py:94:80: E501 line too long (80 > 79 characters) > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > tests/check_acl_log.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tests/check_acl_log.py b/tests/check_acl_log.py > index 1dd9630c0..790846afe 100644 > --- a/tests/check_acl_log.py > +++ b/tests/check_acl_log.py > @@ -91,7 +91,8 @@ def main(): > try: > if parsed_log[key] != val: > print( > - f"Expected log {key}={val} but got {key}={parsed_log[key]} \ > + f"Expected log {key}={val} but got \ > + {key}={parsed_log[key]} \ > in:\n\t'{acl_log}" It's not a problem of this patch, but doesn't '\' introduce a bit number of unnecessary spaces in the string? It should be just split into multiple strings, i.e.: f"Expected log {key}={val} but got " f"{key}={parsed_log[key]} in:\n\t'{acl_log}" (not sure what ' is for in that message) Also, on a side note, f-strings are available only starting with python 3.6, while base requirement for OVN is 3.4. Also not really a problem of this patch. Best regards, Ilya Maximets.
On 8/3/22 13:44, Ilya Maximets wrote: > On 8/3/22 12:39, Dumitru Ceara wrote: >> Starting with flake8>=5.0.1 we got the following report: >> tests/check_acl_log.py:94:80: E501 line too long (80 > 79 characters) >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> tests/check_acl_log.py | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tests/check_acl_log.py b/tests/check_acl_log.py >> index 1dd9630c0..790846afe 100644 >> --- a/tests/check_acl_log.py >> +++ b/tests/check_acl_log.py >> @@ -91,7 +91,8 @@ def main(): >> try: >> if parsed_log[key] != val: >> print( >> - f"Expected log {key}={val} but got {key}={parsed_log[key]} \ >> + f"Expected log {key}={val} but got \ >> + {key}={parsed_log[key]} \ >> in:\n\t'{acl_log}" > > It's not a problem of this patch, but doesn't '\' introduce > a bit number of unnecessary spaces in the string? True, I had mechanically split it too. > > It should be just split into multiple strings, i.e.: > > f"Expected log {key}={val} but got " > f"{key}={parsed_log[key]} in:\n\t'{acl_log}" > Fair point, I can prepare a v2 with this. > (not sure what ' is for in that message) Now that you mention it I think it's either unmatched or stray. I can change it to: f"{key}={parsed_log[key]} in:\n\t'{acl_log}'" What do you think? > > Also, on a side note, f-strings are available only starting > with python 3.6, while base requirement for OVN is 3.4. I'm not sure what to do about this. We can also just give up on f-strings for now. This is the only file where we use them as far as I can tell. Or is it acceptable to bump the base requirement to 3.6? Mark, what do you think? > Also not really a problem of this patch. > > Best regards, Ilya Maximets. > Thanks for the review! Dumitru
diff --git a/tests/check_acl_log.py b/tests/check_acl_log.py index 1dd9630c0..790846afe 100644 --- a/tests/check_acl_log.py +++ b/tests/check_acl_log.py @@ -91,7 +91,8 @@ def main(): try: if parsed_log[key] != val: print( - f"Expected log {key}={val} but got {key}={parsed_log[key]} \ + f"Expected log {key}={val} but got \ + {key}={parsed_log[key]} \ in:\n\t'{acl_log}" ) exit(1)
Starting with flake8>=5.0.1 we got the following report: tests/check_acl_log.py:94:80: E501 line too long (80 > 79 characters) Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- tests/check_acl_log.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)