Skip to content
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

Can Empress._to_dict be made public? #470

Closed
gwarmstrong opened this issue Dec 17, 2020 · 4 comments
Closed

Can Empress._to_dict be made public? #470

gwarmstrong opened this issue Dec 17, 2020 · 4 comments

Comments

@gwarmstrong
Copy link
Member

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 biocore/microsetta-public-api#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
Copy link
Collaborator

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
Copy link
Member

ElDeveloper commented Dec 17, 2020 via email

@gwarmstrong
Copy link
Member Author

gwarmstrong commented Dec 17, 2020

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
Copy link
Collaborator

fedarko commented Dec 22, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants