abrt / retrace-server

Application for remote coredump analysis
GNU General Public License v2.0
40 stars 30 forks source link

Various Pylint fixes and code quality improvements #457

Closed mgrabovsky closed 2 years ago

mgrabovsky commented 2 years ago
codecov-commenter commented 2 years ago

Codecov Report

Merging #457 (cf120bc) into master (b564b55) will increase coverage by 1.33%. The diff coverage is 24.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #457      +/-   ##
==========================================
+ Coverage   20.70%   22.04%   +1.33%     
==========================================
  Files          14       18       +4     
  Lines        2811     2840      +29     
==========================================
+ Hits          582      626      +44     
+ Misses       2229     2214      -15     
Flag Coverage Δ
unittests 22.04% <24.54%> (+1.33%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/retrace/argparser.py 36.84% <0.00%> (ø)
src/retrace/plugins.py 36.17% <0.00%> (ø)
src/retrace/retrace_worker.py 6.52% <8.62%> (+0.28%) :arrow_up:
src/retrace/retrace.py 17.25% <10.89%> (+0.74%) :arrow_up:
src/retrace/archive.py 16.76% <16.76%> (ø)
src/retrace/architecture.py 30.95% <30.95%> (ø)
src/retrace/util.py 34.82% <37.50%> (+0.97%) :arrow_up:
src/retrace/hooks/hooks.py 29.16% <40.00%> (+1.53%) :arrow_up:
src/retrace/logging.py 75.00% <75.00%> (ø)
src/retrace/__init__.py 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b564b55...cf120bc. Read the comment docs.

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert when merging cf120bc95c4eb7e6104bfa0b153a96fbdc74a7ce into b564b552e82182caae3a662e8ca81c0063725dea - view on LGTM.com

new alerts:

mgrabovsky commented 2 years ago

@DaveWysochanskiRH I did a bit of refactoring in these commits. There should be no functional changes, but our capabilities with regards to testing regressions are limited. Could you please check that this does not break anything for you?

DaveWysochanskiRH commented 2 years ago

@mgrabovsky sure I'll give these a test and let you know hopefully in an hour or so.

DaveWysochanskiRH commented 2 years ago

@mgrabovsky task failures across the board with this message, haven't tracked it down yet

[2022-02-15 06:20:48] [I] Analyzing crash data
[2022-02-15 06:20:48] [I] Vmcore size: 60.82 MB
[2022-02-15 06:20:48] [E] Task failed: 'str' object has no attribute 'removesuffix'
DaveWysochanskiRH commented 2 years ago

@mgrabovsky probably some issue in d93d51722614c ?

$ git blame src/retrace/retrace.py | grep removesuffix
d93d51722614c src/retrace/retrace.py (Matěj Grabovský  2022-02-14 15:50:07 +0100  135)                 kernelver_str = kernelver_str.removesuffix(f".{flavour}")
d93d51722614c src/retrace/retrace.py (Matěj Grabovský  2022-02-14 15:50:07 +0100  142)                 kernelver_str = kernelver_str.removesuffix(f".{arch}")
d93d51722614c src/retrace/retrace.py (Matěj Grabovský  2022-02-14 15:50:07 +0100  151)                     self.release = self.release.removesuffix(flavour)
DaveWysochanskiRH commented 2 years ago

@mgrabovsky oh removesuffix is python 3.9 or above. Is there another more backward compatible way? https://docs.python.org/3/library/stdtypes.html#str.removesuffix

DaveWysochanskiRH commented 2 years ago

Ok I think this is an easy fix...

DaveWysochanskiRH commented 2 years ago

Testing this

Author: Dave Wysochanski <dwysocha@redhat.com>
Date:   Tue Feb 15 06:40:04 2022 -0500

    Avoid use of removesuffix to retain earlier Python compatibility

    Use the older code rather than removesuffix which is only
    available with python 3.9 or above:
    https://docs.python.org/3/library/stdtypes.html#str.removesuffix

    Fixes the following error when retracing on any system with
    earlier python (such as RHEL8):
    [E] Task failed: 'str' object has no attribute 'removesuffix'

    Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>

diff --git a/src/retrace/retrace.py b/src/retrace/retrace.py
index 2c9ab08bb6fc..79da8d07ca88 100644
--- a/src/retrace/retrace.py
+++ b/src/retrace/retrace.py
@@ -132,14 +132,14 @@ class KernelVer:
         for flavour in KernelVer.FLAVOUR:
             if kernelver_str.endswith(f".{flavour}"):
                 self.flavour = flavour
-                kernelver_str = kernelver_str.removesuffix(f".{flavour}")
+                kernelver_str = kernelver_str[:-len(flavour) - 1]
                 break

         self._arch = None
         for arch in KernelVer.ARCH:
             if kernelver_str.endswith(f".{arch}"):
                 self._arch = arch
-                kernelver_str = kernelver_str.removesuffix(f".{arch}")
+                kernelver_str = kernelver_str[:-len(arch) - 1]
                 break

         self.version, self.release = kernelver_str.split("-", maxsplit=1)
@@ -148,7 +148,7 @@ class KernelVer:
             for flavour in KernelVer.FLAVOUR:
                 if self.release.endswith(flavour):
                     self.flavour = flavour
-                    self.release = self.release.removesuffix(flavour)
+                    self.release = self.release[:-len(flavour)]
                     break

         self.realtime = "rt" in self.release
DaveWysochanskiRH commented 2 years ago

@mgrabovsky if you squash in that small patch above it looks like everything runs ok.

mgrabovsky commented 2 years ago

@DaveWysochanskiRH Oops, sorry about that – I keep forgetting that these are cutting-edge features. I've reverted those changes now.