Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

Enable LGTM checks and fix few LGTM alerts #302

Merged
merged 5 commits into from
Jan 30, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Jan 29, 2020

What do these changes do?

Improve the codebase to resolve the remaining LGTM's warnings and some of the recommendations.

LGTM itself is enabled via GitHub actions/integrations, so no code changes are needed.

Description

LGMT is a code quality control tool (one of them) — as in "looks good to me": https://lgtm.com/

This project's analysis can be previewed at https://lgtm.com/projects/g/zalando-incubator/kopf/

The code logic is changed in few places — presumably, with no behavioural changes (just try-except-else restructuring to a flat code).

Issues/PRs

Issues: #13

Type of changes

  • Mostly CI/CD automation, contribution experience

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

nolar added 4 commits January 29, 2020 15:17
The logic is supposed to be exactly the same, just
all the try-except-else are rewritten differently
(more straightforward).

This allows to reindent the code better (shorter lines),
remove the unused variables (to make the linters happy),
while keeping is visually symmetric.
This also should resolve some code linting problems in advance.
@nolar nolar added the refactoring Code cleanup without new features added label Jan 29, 2020
@zincr
Copy link

zincr bot commented Jan 29, 2020

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

@zincr
Copy link

zincr bot commented Jan 29, 2020

🤖 zincr found 1 problem , 0 warnings

❌ Approvals
✅ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

@lgtm-com
Copy link

lgtm-com bot commented Jan 29, 2020

This pull request fixes 10 alerts when merging 74ee728 into 3614ee4 - view on LGTM.com

fixed alerts:

  • 6 for Unused local variable
  • 3 for Unnecessary 'else' clause in loop
  • 1 for Except block handles 'BaseException'

`kopf.on` decorators are intentionally duplicated
for better IDE integrations and code hinting.
@lgtm-com
Copy link

lgtm-com bot commented Jan 30, 2020

This pull request fixes 10 alerts when merging 0074b96 into 3614ee4 - view on LGTM.com

fixed alerts:

  • 6 for Unused local variable
  • 3 for Unnecessary 'else' clause in loop
  • 1 for Except block handles 'BaseException'

@nolar nolar assigned aweller and unassigned aweller Jan 30, 2020
@nolar nolar requested a review from aweller January 30, 2020 11:07
@nolar nolar merged commit 6d979a2 into zalando-incubator:master Jan 30, 2020
@nolar nolar deleted the lgtm branch January 30, 2020 15:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
refactoring Code cleanup without new features added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants