pepkit / peppy

Project metadata manager for PEPs in Python
https://pep.databio.org/peppy
BSD 2-Clause "Simplified" License
37 stars 12 forks source link

Prohibit, circumvent or warn about setting object variables which are @properties #190

Open afrendeiro opened 6 years ago

afrendeiro commented 6 years ago

Some attributes of the Project class were changed to a @property. The user should be prohibited (raise error) or warned about this when trying to setattr on this type of attribute to prevent unexpected behavior:

>>> prj = Project("config.yaml")
>>> prj.samples
Out[1]:
[Sample 'ATAC-seq_CD4',
 Sample 'ATAC-seq_CLL',
 Sample 'ATAC-seq_Bcell',
 Sample 'ATAC-seq_NK']
>>> [sample for sample in prj.samples if sample.cell_type == "CD4"]
Out[2]:
[Sample 'ATAC-seq_CD4']
>>> prj.samples = [sample for sample in prj.samples if sample.cell_type == "CD4"]
>>> prj.samples
Out[3]:
[Sample 'ATAC-seq_CD4',
 Sample 'ATAC-seq_CLL',
 Sample 'ATAC-seq_Bcell',
 Sample 'ATAC-seq_NK']

Alternatively, one would catch the call to setattr for these and set instead the parent attribute (in this case _samples). In such a case, what would be the benefit of using @property compared to a normal mutable attribute?

vreuter commented 6 years ago

I think you're exactly right, that putting in a hook to set the parent attribute would defeat the purpose of the property annotation. I think that change was made (sorry, probably by me!) with the thought that a Project's samples shouldn't be allowed to change, that they should be defined in the annotation sheet and then able to provide a filtered collection of samples (there's currently a way to do this by protocol), but not actually modified. I guess we can eliminate @property and have samples be an ordinary attribute if that's the more favorable usage model, I just did it this way for what felt safer I think.

afrendeiro commented 6 years ago

I understand the purpose and wouldn't particularly mind it as long as there wouldn't be surprises like above. If no immediate added value, maybe it is better to switch back to ordinary attributes.