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

Split handers & callbacks types for resource-related handlers #329

Merged
merged 4 commits into from
Mar 18, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Mar 12, 2020

What do these changes do?

Move the code around and rename some types & classes for callbacks & handlers.
No user-facing changes.

Description

This is the last preparing PR before the daemons & timers PR (#330).

Here, we just move the code around and rename classes. It would be messy to have these in the daemons & timers PR, so it goes separately.

Specifically, the resource-related handlers are split into resource-changing & resource-watching — same as it is already done for registries. These handlers & callbacks are de factor different anyway, but were declared as the same for laziness reasons. In the daemons & timers PR, also resource-spawning handlers & daemon/timer callbacks will be added with their own signatures.

No behavioural changes, no user-facing changes.

Issues/PRs

Issues: #19

Type of changes

  • Refactoring (non-breaking change which does not alter the behaviour)

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 nolar added the refactoring Code cleanup without new features added label Mar 12, 2020
@zincr
Copy link

zincr bot commented Mar 12, 2020

🤖 zincr found 0 problems , 1 warning

ℹ️ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

  • ℹ️ kopf/reactor/registries.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
     

@zincr
Copy link

zincr bot commented Mar 12, 2020

🤖 zincr found 1 problem , 1 warning

❌ 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
     

Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

  • ℹ️ kopf/reactor/registries.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
     

@lgtm-com
Copy link

lgtm-com bot commented Mar 12, 2020

This pull request introduces 1 alert when merging 419c3f6 into de23abe - view on LGTM.com

new alerts:

  • 1 for Unused import

nolar added 3 commits March 13, 2020 20:08
Same as the respective registries and causes are already specialised.
Needed for extra typing, and to introduce additional kind of handlers.
These classes and modules are deprecated anyway, so there is no neeed
to keep them in a nice state, and make it the right way. Only minimally
sufficient fix is enough to make it both work and pass the type-checks.
@nolar nolar force-pushed the split-handers-types branch from 419c3f6 to 194c33c Compare March 13, 2020 19:22
@nolar nolar requested review from 3abdelazim and haikoschol March 13, 2020 19:23
@lgtm-com
Copy link

lgtm-com bot commented Mar 13, 2020

This pull request introduces 1 alert when merging 194c33c into f70e556 - view on LGTM.com

new alerts:

  • 1 for Unused import

haikoschol
haikoschol previously approved these changes Mar 16, 2020
The handlers return coroutines, not the results directly. With some
handlers (such as daemons soon), there could be specialised protocols
with slightly different signatures (kwargs types).
@nolar
Copy link
Contributor Author

nolar commented Mar 16, 2020

@haikoschol Ouch. One more time plz (with a new commit)?

@haikoschol
Copy link

@haikoschol Ouch. One more time plz (with a new commit)?

Sorry about the delay. I saw it yesterday or so but then forgot about it again.

@nolar nolar merged commit 6f702ef into zalando-incubator:master Mar 18, 2020
@nolar nolar deleted the split-handers-types branch March 18, 2020 20:31
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