Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Handler subclassable #130

Merged
merged 1 commit into from
May 15, 2019
Merged

Conversation

bmbouter
Copy link
Member

@bmbouter bmbouter commented May 13, 2019

The Handler object was not very subclassable, so this improves it in a
few ways.

  • hardcoded Handler calls are no longer used
  • classmethods replace staticmethods
  • the HOP_BY_HOP constant is now available on the object
  • add a DISTRIBUTION_MODEL constant that subclasses can declare their
    corresponding distribution model type. If declared only that model
    type will be matched. If unspecified, BaseDistribution will be used.

Unrelated to the subclassing an error case was removed which would
silence all exceptions to the log which is not a good behavior.

https://pulp.plan.io/issues/4719
closes #4719

bmbouter pushed a commit to bmbouter/pulpcore-plugin that referenced this pull request May 13, 2019
*breaking change!*

The core now provides a handler called `BaseHandler` that is designed
for subclassing. This replaces the `Handler` object for plugin-api use.

This also updates the release notes since this is a breaking change.

Required PR: pulp/pulpcore#130

https://pulp.plan.io/issues/4719
closes #4719
@bmbouter bmbouter changed the title Refactor to introduce BaseHandler Make Handler subclassable May 14, 2019
@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #130 into master will increase coverage by 0.1%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #130     +/-   ##
=========================================
+ Coverage   67.07%   67.18%   +0.1%     
=========================================
  Files          64       64             
  Lines        3028     3029      +1     
=========================================
+ Hits         2031     2035      +4     
+ Misses        997      994      -3
Impacted Files Coverage Δ
pulpcore/content/handler.py 62.76% <76.92%> (+1.8%) ⬆️

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 641f5e5...a42ef95. Read the comment docs.

pulpcore/content/handler.py Show resolved Hide resolved
pulpcore/content/handler.py Outdated Show resolved Hide resolved
pulpcore/content/handler.py Outdated Show resolved Hide resolved
pulpcore/content/handler.py Outdated Show resolved Hide resolved
pulpcore/content/handler.py Outdated Show resolved Hide resolved
The `Handler` object was not very subclassable, so this improves it in a
few ways.

- hardcoded `Handler` calls are no longer used
- classmethods replace staticmethods
- the HOP_BY_HOP constant is now available on the object
- add a DISTRIBUTION_MODEL constant that subclasses can declare their
  corresponding distribution model type. If declared only that model
  type will be matched. If unspecified, `BaseDistribution` will be used.

Unrelated to the subclassing an error case was removed which would
silence all exceptions to the log which is not a good behavior.

https://pulp.plan.io/issues/4719
closes pulp#4719
@bmbouter
Copy link
Member Author

@gmbnomis I pushed my changes from your review.

@gmbnomis
Copy link
Contributor

LGTM now. I just set distribution_model in the custom handler in pulp_cookbook and it works as expected. Thank you for implementing this feature!

@bmbouter bmbouter merged commit 42c1d62 into pulp:master May 15, 2019
@bmbouter bmbouter deleted the refactor-handler branch May 15, 2019 20:54
gmbnomis added a commit to pulp/pulp_cookbook that referenced this pull request May 15, 2019
Using the `distributon_model` attribute introduced in
pulp/pulpcore#130, restrict the
pulp_cookbook content app to CookbookDistributions only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants