Skip to content

Conversation

@charettes
Copy link
Contributor

Description

The first commit adds regression tests for the existing metadata generation through SimpleMetadata.

The second one adds a get_metadata() method to the Field class in order to allow fields to specify their own metadata. Backward compatibility is maintained and the SimpleMetadata's API is unchanged, it simply delegates to Field.get_metadata().

This is a change required in order to offer a backward compatible deprecation path for #4021.

@lovelydinosaur
Copy link
Contributor

The footprint for this is defiantly too large in comparison to the small benefit it'd give us.

@charettes
Copy link
Contributor Author

Do you think it might be worth merging the regression tests at least?

@xordoquy
Copy link
Contributor

I find the idea -the original proposal- pretty interesting.
@tomchristie what about keeping things as they currently are and add a call to the field's metadata if it exists ?

Something such as metadata.py:

def get_field_info(self, field):
    if hasattr(field, 'get_metadata'):
        return field.get_metadata()
    else:
        # keep the current code

Therefore we'll have a hook to customize the metadata easily on a per field basis while keeping the footprint reasonable.

@xordoquy
Copy link
Contributor

@charettes will try to take some time to dig that topic - keeping the tests.

@lovelydinosaur
Copy link
Contributor

@tomchristie what about keeping things as they currently are and add a call to the field's metadata if it exists?

I think I'd still consider that API-creep. In this area I'd prefer us to keep the API surface area low. If there's complexity, let's force that to be in the metadata class.

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.

3 participants