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

Adding free-text extension. #655

Merged
merged 6 commits into from
Jul 19, 2024

Conversation

rhysrevans3
Copy link
Contributor

@rhysrevans3 rhysrevans3 commented Apr 10, 2024

Related Issue(s):

Description:

Adding the free-text extension

PR Checklist:

  • pre-commit hooks pass locally
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
  • Changes are added to the CHANGELOG.

@rhysrevans3
Copy link
Contributor Author

@m-mohr Could I get your opinion on this I know you've helped with the extension definition.

@vincentsarago
Copy link
Member

where are we with this PR? @rhysrevans3

@rhysrevans3
Copy link
Contributor Author

where are we with this PR? @rhysrevans3

We're unsure on if basic and advanced search are compatible stac-api-extensions/freetext-search#10

I think the current options are, update the advanced search to better match with the basic search or separate them into two extensions.

@vincentsarago
Copy link
Member

Dear @rhysrevans3 and @m-mohr I've refactored a bit this extension to:

@vincentsarago vincentsarago marked this pull request as ready for review July 18, 2024 11:48
@m-mohr
Copy link
Contributor

m-mohr commented Jul 18, 2024

  • assume that the param for both GET/POST are of string form

Correct for Advanced, not for Basic. See also stac-api-extensions/freetext-search#11

@vincentsarago
Copy link
Member

I guess I'm going to create 2 Extensions to make this a bit more clear

@m-mohr
Copy link
Contributor

m-mohr commented Jul 18, 2024

That's a very good idea, especially as they are not quite compliant yet.

@jonhealy1
Copy link
Collaborator

There is a draft pr in sfeos related to this but I don't think it gives any new information - https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/pull/227/files

@vincentsarago
Copy link
Member

This is ready for review

It would be nice to add documentation but this can be done in another PR ;-)

@m-mohr
Copy link
Contributor

m-mohr commented Jul 18, 2024

Was the implementation tested against STAC Browser by any chance?

@vincentsarago
Copy link
Member

Was the implementation tested against STAC Browser by any chance?

no, but I don't know any backend supporting the free-text spec 😓

@vincentsarago
Copy link
Member

Was the implementation tested against STAC Browser by any chance?

@m-mohr I don't really see how the free-text extension is used by STAC-Browser

maybe https://github.com/radiantearth/stac-browser/blob/108947ebe8cbd64c7e288bbaa3c5f81293706d3e/src/components/SearchFilter.vue#L141 seems to point that q should be an array 🤷 but then I don't know if it's used in a GET or POST request

@vincentsarago
Copy link
Member

I'm going to merge this PR (so I can continue on the collection-search which depends on the free-text ext) but feel free to comment and we could still make changes before we do a next release

@vincentsarago vincentsarago merged commit fe4d0df into stac-utils:main Jul 19, 2024
7 checks passed
@rhysrevans3 rhysrevans3 deleted the freetext-search-ext branch August 21, 2024 07:28
jonhealy1 pushed a commit to stac-utils/stac-fastapi-elasticsearch-opensearch that referenced this pull request Aug 31, 2024
**Description:**

Adding the free-text search extension. Related to
stac-utils/stac-fastapi#655

**PR Checklist:**

- [x] Code is formatted and linted (run `pre-commit run --all-files`)
- [x] Tests pass (run `make test`)
- [x] Documentation has been updated to reflect changes, if applicable
- [x] Changes are added to the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants