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

CUMULUS-3856 Update cumulus-api document #360

Merged
merged 18 commits into from
Jan 30, 2025

Conversation

tclark-innovim
Copy link
Contributor

Addresses issues in CUMULUS-3856

jennyhliu

This comment was marked as resolved.

@jennyhliu jennyhliu added PR Feedback and removed Needs Review Looking for a reviewer labels Jan 18, 2025
@tclark-innovim
Copy link
Contributor Author

@jennyhliu Thanks for the feedback, I will review and make code updates

@Nnaga1
Copy link
Contributor

Nnaga1 commented Jan 21, 2025

one I think you might have missed:

https://nasa.github.io/cumulus-api/unreleased/#migration-counts

@charleshuang80
Copy link
Contributor

Sorry to jump into this late, I wanted to look into this and provide some feedback but was focused on my other ticket. My one suggestion for now is to update the description for prefix and infix in the table in the intro. Because we're switching from ES to PG style queries, these queries don't quite have the same functionality. And if you do a search for infix and prefix in the core code for the feature branch, we are generally just using whereLike on specific fields. So I don't think all the fields listed are being used, and startsWith no longer applies.
There may be one other suggestion I have but I'll need to look through some notes to try and remember if it is something that needs to be addressed.

@tclark-innovim
Copy link
Contributor Author

one I think you might have missed:

https://nasa.github.io/cumulus-api/unreleased/#migration-counts

I removed the Migration Counts section.

@tclark-innovim
Copy link
Contributor Author

Sorry to jump into this late, I wanted to look into this and provide some feedback but was focused on my other ticket. My one suggestion for now is to update the description for prefix and infix in the table in the intro. Because we're switching from ES to PG style queries, these queries don't quite have the same functionality. And if you do a search for infix and prefix in the core code for the feature branch, we are generally just using whereLike on specific fields. So I don't think all the fields listed are being used, and startsWith no longer applies. There may be one other suggestion I have but I'll need to look through some notes to try and remember if it is something that needs to be addressed.

@charleshuang80 I am looking through the code in the feature/es-phase-2 branch, and I see that infix and prefix are still being used in the queries. I checked these object types and fields, and they are still being searched. Let me know if you see something different.

Example:
image

@tclark-innovim
Copy link
Contributor Author

@jennyhliu @Nnaga1 @charleshuang80 I have implemented the feedback that I have received on this PR. Can you review again? Thanks!

Copy link
Contributor

@jennyhliu jennyhliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@npauzenga npauzenga merged commit 46866a7 into master Jan 30, 2025
3 checks passed
@npauzenga npauzenga deleted the teclark/CUMULUS-3856-update-documentation branch January 30, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants