peppy icon indicating copy to clipboard operation
peppy copied to clipboard

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

Open afrendeiro opened this issue 7 years ago • 2 comments

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?

afrendeiro avatar Oct 30 '17 16:10 afrendeiro

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.

vreuter avatar Oct 30 '17 21:10 vreuter

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.

afrendeiro avatar Oct 31 '17 17:10 afrendeiro