Avoid collisions between plugin models #259
Conversation
fao89
force-pushed
the
4681-related-name
branch
from
c3f5ad6
to
5181e5e
Compare
fao89
force-pushed
the
4681-related-name
branch
from
5181e5e
to
1730ee4
Compare
Codecov Report
@@ Coverage Diff @@
## master #259 +/- ##
==========================================
- Coverage 67.6% 66.59% -1.02%
==========================================
Files 69 69
Lines 3260 3272 +12
==========================================
- Hits 2204 2179 -25
- Misses 1056 1093 +37
Continue to review full report at Codecov.
|
CHANGES/4681.bugfix
Outdated
| @@ -0,0 +1 @@ | |||
| Avoid collisions between plugin models. | |||
There was a problem hiding this comment.
This is good for user-facing changelog IMO, but we need something for plugin writers that indicates that this is a breaking change. One solution would be adding a .feature CHANGE pointing to this issue in pulp/pulpcore-plugin#119
There was a problem hiding this comment.
Even though there are no code changes to pulpcore-plugin, adding a 4681.feature to pulpcore-plugin's changelog makes sense to me. I think this would be a great way to document changes to the plugin API.
cc @bmbouter
There was a problem hiding this comment.
@daviddavis I agree, even though the changes occur here, we should represent this in the pulpcore-plugin release notes instead. I think of the changelog for pulpcore as the changelog for end users, and the changelog for pulpcore-plugin as the changelog for plugin writers.
@asmacdo, I don't think end-users will understand 4681.feature for the reasons above. For that reason removing 4681.feature and keeping 4681.bugfix make the most sense to me.
There was a problem hiding this comment.
Should we rename this file to 4681.misc then?
pulpcore/app/models/base.py
Outdated
| class MasterModelMeta(ModelBase): | ||
| def __new__(cls, name, bases, attrs, **kwargs): | ||
| """Override __new__ to set the default_related_name.""" | ||
| if Model not in bases and MasterModel not in bases: |
There was a problem hiding this comment.
Could you add a comment explicitly stating that this only affects "detail" models?
fao89
force-pushed
the
4681-related-name
branch
from
1730ee4
to
f2e3167
Compare
pulpcore/app/models/base.py
Outdated
| abstract = getattr(meta, "abstract", None) | ||
|
|
||
| if not default_related_name and not abstract: | ||
| raise Exception(_("The default_related_name option has not been set for " |
There was a problem hiding this comment.
Maybe put single quotes around default_related_name so it's clearer for the user?
CHANGES/4681.feature
Outdated
| @@ -0,0 +1,2 @@ | |||
| Breaking change on how to subclass Master/Detail models in plugins. | |||
| Please refer to https://github.com/pulp/pulpcore-plugin/pull/119 | |||
There was a problem hiding this comment.
I think you can drop this file based on the above conversation.
fao89
force-pushed
the
4681-related-name
branch
from
f2e3167
to
e34159e
Compare
Making mandatory declaring default_related_name Required PR: pulp/pulp_file#269 Required PR: pulp/pulp-certguard#27 closes #4681
fao89
force-pushed
the
4681-related-name
branch
from
e34159e
to
75f7916
Compare
Making mandatory declaring default_related_name
closes #4681
https://pulp.plan.io/issues/4681
NOTE: DO NOT MERGE UNTIL AUGUST 19, 2019