Skip to content

Add dataset to data concepts classes#278

Merged
asantas93 merged 8 commits intomasterfrom
feature/dataset-on-data-concepts
Apr 6, 2020
Merged

Add dataset to data concepts classes#278
asantas93 merged 8 commits intomasterfrom
feature/dataset-on-data-concepts

Conversation

@asantas93
Copy link
Copy Markdown
Contributor

@asantas93 asantas93 commented Apr 3, 2020

Citrine Python PR

Description

Readies client for new dataset field which will appear on all data concepts classes, if they were created from the backend's response.

PR Type:

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Adherence to team decisions

  • I have added tests for 100% coverage
  • I have written Numpy-style docstrings for every method and class.
  • I have communicated the downstream consequences of the PR to others.

@asantas93 asantas93 changed the title WIP datasets on data concepts Add dataset to data concepts classes Apr 3, 2020
Copy link
Copy Markdown
Contributor

@maxhutch maxhutch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sensible. One naming request

Comment thread src/citrine/resources/data_concepts.py Outdated
raise TypeError(
"{} must be a dictionary or None but was {}".format(field, value))
deserialized = clazz.build(value)
elif issubclass(clazz, Property):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the namespace collision with the property attribute, could we use this as properties.Property (as we have in some other places?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is actually a python syntax error, surprisingly. I could give it an alias though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anything to disambiguate it

@asantas93 asantas93 requested a review from maxhutch April 3, 2020 21:24
maxhutch
maxhutch previously approved these changes Apr 5, 2020
Copy link
Copy Markdown
Contributor

@maxhutch maxhutch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@asantas93 asantas93 merged commit 50d5f3c into master Apr 6, 2020
@asantas93 asantas93 deleted the feature/dataset-on-data-concepts branch April 6, 2020 16:41
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.

2 participants