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

refactor ingest_spectral_types function #539

Merged

Conversation

Exu-112
Copy link
Collaborator

@Exu-112 Exu-112 commented Jul 16, 2024

Short description: refactor ingest_spectral_types function to ingest one at a time and use the new ingest method

Link to relevant issue: Closes #481

@kelle kelle mentioned this pull request Jul 23, 2024
4 tasks
@kelle kelle requested a review from dr-rodriguez July 24, 2024 20:07
@kelle
Copy link
Collaborator

kelle commented Jul 24, 2024

Please take a look. If all is okie dokie, I'll add the 3863 modified JSON files with the new photometric column.

@dr-rodriguez
Copy link
Collaborator

Overall, this gets the job done. I think we can do better in terms of the function flow and styling. One of the things I see a lot of these days is giant functions that do more than one thing, which is bad design and hard to test. One way we could work towards catching that is implementing a linter to this project. I'm using ruff with some custom settings in one of my own projects and the McCabe complexity indicator is useful for this: https://docs.astral.sh/ruff/rules/complex-structure/

I recommend creating a few issues from the comments above and then we can approve and update the json files.

@kelle
Copy link
Collaborator

kelle commented Jul 24, 2024

Excellent suggestions. I implemented most of them.

@kelle kelle requested a review from dr-rodriguez July 24, 2024 22:41
@kelle kelle merged commit 5f6d6d3 into SIMPLE-AstroDB:main Jul 25, 2024
1 check passed
This was referenced Jul 25, 2024
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

Successfully merging this pull request may close these issues.

Refactor ingest_spectral_types function
3 participants