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

Standardize discovery module #688

Closed
AdamTheisen opened this issue Jun 19, 2023 · 7 comments · Fixed by #737
Closed

Standardize discovery module #688

AdamTheisen opened this issue Jun 19, 2023 · 7 comments · Fixed by #737
Assignees
Labels

Comments

@AdamTheisen
Copy link
Collaborator

We need to standardize the discovery module. I think it would be better if we remove the "get_" from every file and leave as "neon", "arm", etc... We then should update the functions to remove "get".

@zssherman
Copy link
Collaborator

zssherman commented Jun 20, 2023

@AdamTheisen So rename the modules and then for those functions that don't start with download, but instead with get, change those to download_blank as well?

@zssherman zssherman self-assigned this Jun 20, 2023
@AdamTheisen
Copy link
Collaborator Author

AdamTheisen commented Jun 20, 2023

@zssherman I'm thinking of removing the "get_" from the file name as the main part. In looking at the function names, we have to be careful and maybe we make it a standard that if it actually downloads a file, we have "download_" at the front instead of "get_" but if all it's doing but if it's returning a DataSet then keep as "get_". Open to thoughts/opinions hear though

Looking at IO, we just have mpl, neon, armfiles (which we might want to rename as well to just arm), icartt, etc... so just trying to be consistent in discovery and with the rest of ACT

@AdamTheisen
Copy link
Collaborator Author

I was also thinking that we might want to follow similar suit to the renaming of HistogramDisplay to DistributionDisplay and copy the codes over to the new name, put in a deprecation warning and then remove in the next major release.

@zssherman
Copy link
Collaborator

@AdamTheisen Yeah I think that's a good path forward

@zssherman
Copy link
Collaborator

zssherman commented Jun 22, 2023

Deprecation warning i think is a good idea as well to warn people. We could start with one submodule at a time, can change the discover module and have the warning in the original code and then add the duplicates of each module with the renamed file and functions (if functions need renaming). And do renaming of the io stuff later on. The docstrings in the original code might need to be removed so were still not generating the old code, and have the docs build the new stuff (this part im unsure though to be honest)

@zssherman
Copy link
Collaborator

zssherman commented Jun 22, 2023

But I think i get what you mean as well on the function naming, as airnow, its just getting the information from the website and is not actually downloading anything

@AdamTheisen
Copy link
Collaborator Author

The base for this is in #724 but we will need to update when we do the V2.0.0 release to remove the old files and make sure the new ones are in init.py

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

Successfully merging a pull request may close this issue.

2 participants