monarch-initiative / genophenocorr

Genotype Phenotype Correlation
https://monarch-initiative.github.io/genophenocorr/stable
MIT License
4 stars 1 forks source link

method names should be unambiguous #59

Open pnrobinson opened 9 months ago

pnrobinson commented 9 months ago

Can we review the names of methods in the package? For instance, if I see a method called all_transcripts, then I might expect to get a list or set of Transcript objects. If we change the name of the method to get_all_transcript_ids, then it is clearer that (1) I am getting something (2) that I am getting identifiers rather than whatever else a Transcript might be.

@ielis I am not sure if it isn't better to restrict use of @property to simple attributes rather than things we need to compute?

e.g.

@property
    def all_transcripts(self):
        """
        Returns:
            set: A set of all the transcript IDs in the Cohort
        """
        all_trans = set()
        for var in self.all_variants:
            all_trans.update([trans.transcript_id for trans in var.tx_annotations])
        return all_trans

could be

    def get_transcript_ids(self):
        """
        :returns:  A set of all the transcript IDs in the Cohort
        :rtype: Set[str]
        """
        all_trans = set()
        for var in self.all_variants:
            all_trans.update([trans.transcript_id for trans in var.tx_annotations])
        return all_trans

I think the assumption is that we get "all" with a method call like this? No need to have "all" in the name unless we have a similar "some" method?

ielis commented 8 months ago

Yes, the methods on Cohort need typing annotations, better docs, and better names. The method names are important, because good names make API unsurprising, and, therefore, easier to grok.

Yes, I agree with the strategy regarding property vs. method.