search

biocore / empress

A fast and scalable phylogenetic tree viewer for microbiome data analysis
BSD 3-Clause "New" or "Revised" License
48 stars 31 forks source link

Can Empress._to_dict be made public? #470

Closed gwarmstrong closed 3 years ago

gwarmstrong commented 3 years ago

The empress.core.Empress._to_dict method is really handy for serializing an empress object, and we would like to do this on https://github.com/biocore/microsetta-public-api/ .

As noted by @wasade on https://github.com/biocore/microsetta-public-api/pull/84 it is not great to call a private method. Is there any reason this method should not be used as, or made into a public method of the core.Empress object?

kwcantrell commented 3 years ago

I don't see any issue with making Empress._to_dict() public. The original purpose of the empress python object was to create the html page so there was no need to expose the Empress._to_dict() function as using it would be outside the scope of the object. However, it might be better to create a wrapper function for it as Empress._to_dict() returns a dict which contains references to internal variables. So before we return the dict, we would want to duplicated the objects first.

ElDeveloper commented 3 years ago

+1, this is a good idea and there shouldn't be any issues with that.

On (Dec-17-20|12:48), kwcantrell wrote:

I don't see any issue with making Empress._to_dict() public. The original purpose of the empress python object was to create the html page so there was no need to expose the Empress._to_dict() function as using it would be outside the scope of the object. However, it might be better to create a wrapper function for it as Empress._to_dict() returns a dict which contains references to internal variables. So before we return the dict, we would want to duplicated the objects first.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://urldefense.com/v3/__https://github.com/biocore/empress/issues/470*issuecomment-747693137__;Iw!!Mih3wA!XsFXxWIF_3dQXd1OlUPlZlngiF-O9WISdHXiHHf_51r3gulMfemtt45ciIj_kV4$

gwarmstrong commented 3 years ago

So before we return the dict, we would want to duplicated the objects first.

Maybe something like a copy=True parameter? For example my application never needs to modify the dict, so copying is not necessary and a little wasteful.

fedarko commented 3 years ago

Looks like #472 addressed this, so I think we can close this (pls yell at me if not)