[sos-devel] [announce] LGTM reviews for sos PRs

[Pavel Moravec]

On Wed, Feb 6, 2019 at 5:46 PM Bryn M. Reeves <bmr at redhat.com> wrote:

> Hi Folks,
>
> I've experimentally turned on automatic LGTM reviews for sos pull
> requests.
>
> LGTM is a static code analysis suite for Python that produces a
> report on possible errors and language misuse. You can view the
> current report for sos master here:
>
>   https://lgtm.com/projects/g/sosreport/sos/
>
> There are roughly 50 currently, and on a quick scan most are an
> actionable bug (although some with very low/no impact). We'll
> be working through these over the next few weeks, although feel
> free to submit PRs for any outstanding items you notice.
>
> If the reviews seem useful we'll start to incorporate them in
> the routine sos review process.
>
> LMK if it LGTY!
>
> Regards,
> Bryn.
>

Nice and useful initiative, thanks for that!

I started to review the warnings, but I feel some of them are red herrings:

1) Both errors
https://lgtm.com/projects/g/sosreport/sos/alerts/?mode=list&severity=error
does not treat a r.e. in string
like """
     ^
     ..
     $
     """

properly, as I think they treat the leading whitespace before ^ and before
closing """ as valid chars (they arent)


2) for-else construction at
https://lgtm.com/projects/g/sosreport/sos/snapshot/904808a29893d182621cf32cb9074b50f817b9a1/files/sos/plugins/__init__.py?sort=name&dir=ASC&mode=heatmap#L517
is also valid - "return" _is_ kind of break :)


3) dead code at
https://github.com/sosreport/sos/blob/master/sos/sosreport.py#L126 revealed
the fact no xml reports are generated at all :) (self.enabled = False all
the time) - since this is the case for years, doesn't it make sense to
remove the XmlReport completely? (that would get rid of 2 other warnings)


For the 3) and many valid warnings, I raised #1559 now.

Kind regards,
Pavel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/sos-devel/attachments/20190207/498202e6/attachment.htm>