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

MAST: Tesscut doc needs updating, cloud API is no-op #3097

Closed
pllim opened this issue Sep 23, 2024 · 3 comments · Fixed by #3113
Closed

MAST: Tesscut doc needs updating, cloud API is no-op #3097

pllim opened this issue Sep 23, 2024 · 3 comments · Fixed by #3113

Comments

@pllim
Copy link
Member

pllim commented Sep 23, 2024

As requested by @orionlee in #1521 (comment)

Public documentation should probably be updated, as TessCut class still have the documented
enable_cloud_dataset() and disable_cloud_dataset() methods that are essentially no-ops.

@bsipocz
Copy link
Member

bsipocz commented Sep 23, 2024

cc @snbianco

@snbianco snbianco self-assigned this Sep 23, 2024
@snbianco
Copy link
Contributor

Looks like the issue here is that enable_cloud_dataset and disable_cloud_datset are defined on the base class of TesscutClass, which is MastQueryWithLogin.

Some options:

  1. Override the functions in TesscutClass, issue a deprecation warning, and add a :private directive to hide them from the docs.
  2. Override the functions in TesscutClass and raise an exception.
  3. Move the functions into the child classes excluding TesscutClass.

I'm leaning towards Option 1, but I wanted to check if there is a standard already in place for this type of thing.

@pllim
Copy link
Member Author

pllim commented Sep 26, 2024

If these are used in child classes, you could also raise NotImplementedError in the base class with a message that it should be defined in child classes. Ultimately, it is up to you, as I am unfamiliar with MAST API internals. Thanks!

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

Successfully merging a pull request may close this issue.

3 participants