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

Change constructs access API for consistency #132

Closed
davidhassell opened this issue Apr 7, 2021 · 0 comments · Fixed by #138
Closed

Change constructs access API for consistency #132

davidhassell opened this issue Apr 7, 2021 · 0 comments · Fixed by #138
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@davidhassell
Copy link
Contributor

davidhassell commented Apr 7, 2021

Current situation

The construct and construct_key methods take a single positional identity argument (https://github.com/NCAS-CMS/cfdm/blob/v1.8.8.0/cfdm/mixin/constructaccess.py#L301):

    def construct(self, identity, default=ValueError()):

Proposed change

The construct and construct_key methods take any number of positional arguments, as well as new keyword arguments:

    def construct(self, *identity, default=ValueError(), **filter_kwargs):

The **filter_kwargs are passed to the new Constructs.filter method for further selection flexibility.

This makes sense, as you can already match on multiple identities by providing a re.Pattern object, so it doesn't makes sense to restrict this to, say, *["latitude", "longitude"]

Consequences

The only possibly bad effect that this could have is for the default argument to be consumed by the *identity arguments if it is given as a positional parameter. For example, to Specify a default of None:

>>> f.construct('altitude', None)  # ok at v1.8.8.0
>>> f.construct('altitude', None)  # NOT ok with proposed change
>>> f.construct('altitude', default=None)  # ok with proposed change and at v1.8.8.0

Regardless of this potential for code breaking, I think this change will be beneficial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant