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

Attributes aren't inherited by subclasses #140

Closed
rhunwicks opened this issue Sep 8, 2014 · 5 comments
Closed

Attributes aren't inherited by subclasses #140

rhunwicks opened this issue Sep 8, 2014 · 5 comments

Comments

@rhunwicks
Copy link
Contributor

I have a base model and a set of subclasses of the model. As you might expect, when I create the import/export ModelResources for those subclass models there is a lot of duplicated code related to the base model. I'd like to move that code into a base ModelResource and then subclass it. For example:

class BaseResource(resources.ModelResource):
    parent_field = fields.Field(attribute='parent__base_field', readonly=True)

    class Meta:
        model = BaseModel
        use_transactions = True 
        skip_unchanged = True
        report_skipped = True
        abstract = True

class SubclassResource(BaseResource)

    class Meta:
        model = SubclassModel

Unfortunately, when I tried this the fields.Field attributes on the parent BaseResource aren't inherited on the SubclassResource. I think this is because the DeclarativeMetaclass.__new__() only copies attributes passed in directly and doesn't loop through bases and add them from there. However, I'm not familiar enough with metaclass programming or django-import-export to know the best way to fix it.

@bmihelac
Copy link
Member

bmihelac commented Sep 8, 2014

I think there is an PR that I think related to this:

https://github.com/bmihelac/django-import-export/pull/123

Specific code:
https://github.com/bmihelac/django-import-export/pull/123/files#diff-c4f0017c20379df2e96867bdcc1a42b7R114

However for that pull request misses tests and have some unrelated commits.

@rhunwicks
Copy link
Contributor Author

I'm working on a replacement PR to solve this issue, which doesn't have any of the other stuff. It also allows Meta attributes to be inherited so that skip_unchanged etc can be set on a parent class, as well as fields.

One thing I'd like you to consider... what do we do with unauthorized Meta attributes?

For example, I have a custom Resource where I did:

class BaseResource(resources.ModelResource):
    parent_field = fields.Field(attribute='parent__base_field', readonly=True)

    class Meta:
        model = BaseModel
        insert_only = True  # This is a custom attribute

If I change the way the _meta is setup, what should I do with attributes that exist in the subclasses, but which aren't official - i.e. which don't exist in ResourceOptions.

I see three approaches:

  1. We discard any unofficial attributes, possibly issuing a warning. This might break existing code (like mine) but would be consistent with how Django deals with additional attributes on Meta classes in models.
  2. We discard any unofficial attributes on parent classes but allow them on the final class. This would be an intermediate solution, where we exert more control over Meta but don't break any existing code, because it can't be relying on inheritance yet. The documentation would need to be clear.
  3. We continue to allow custom attributes in Meta and faithfully copy them to the subclasses.

Thoughts?

@bmihelac
Copy link
Member

@rhunwicks I am not sure. Would like to follow Django practices to reduce possibility of surprises. How does other libraries handle this?

@rhunwicks
Copy link
Contributor Author

I'm not aware of any other libraries that are doing Meta inheritance. I would lean towards copying the attributes because:

  • it will mean existing code still works
  • I have found the Django models approach inconvenient anyway
  • we don't have much functionality in our metaclass, which is I think the justification for Django models removing them - they don't want to take the chance a user-supplied attribute breaks something because the metaclass methods don't know about it.

rhunwicks added a commit to rhunwicks/django-import-export that referenced this issue Apr 22, 2015
rhunwicks added a commit to rhunwicks/django-import-export that referenced this issue Apr 22, 2015
bmihelac added a commit that referenced this issue Apr 22, 2015
@halfnibble
Copy link

Please disregard my previous comments. I had somehow managed to install my packages using an old requirements.txt file. Everything works as expected. Thanks.

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

No branches or pull requests

3 participants