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

Add default columns and do not save them to disk #440

Merged
merged 7 commits into from Nov 8, 2017

Conversation

Projects
None yet
2 participants
@nickhand
Member

nickhand commented Nov 7, 2017

This fixes #431.

  • introduces "default" columns --> these are columns that are defined via class member functions
  • add a metaclass to CatalogSource that picks up default columns and hard coded columns that are part of the class
  • add is_default attribute to ColumnAccessor, letting users test if a column is default; I thought this was better than a defaults list, which would change as columns are overriden
  • filter out and do not save defaults in CatalogSource.save()
daskarray : dask.array.Array
the column in dask array form
is_default : bool, optional
whether this column is a default column

This comment has been minimized.

@rainwoodman

rainwoodman Nov 7, 2017

Member

Add a comment (thus shall not be serialized)

name : str, optional
the name of the column; if not provided, the name of the function
being decorated is used
default : bool, optional

This comment has been minimized.

@rainwoodman

rainwoodman Nov 7, 2017

Member

I wonder if the name 'fallback' is better suited than default in this context.

This comment has been minimized.

@nickhand

nickhand Nov 7, 2017

Member

I went with "is_default" here and below to unify the syntax. I think we are now using "is_default" everywhere so the syntax reads "column.is_default"

getter.column_name = name
def decorator(getter, name=name):
getter.column_name = getter.__name__ if name is None else name
getter.default_column = default

This comment has been minimized.

@rainwoodman

rainwoodman Nov 7, 2017

Member

is_default_column?

@nickhand

This comment has been minimized.

Member

nickhand commented Nov 7, 2017

I think this is ready to merge, @rainwoodman ?

@rainwoodman

This comment has been minimized.

Member

rainwoodman commented Nov 8, 2017

I am OK with this too.

@nickhand nickhand merged commit 8585045 into master Nov 8, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 95.289%
Details

@nickhand nickhand deleted the default-columns branch Nov 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment