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

Add support to search for web features in searchcache [Part 3/4] #3665

Merged
merged 8 commits into from
Jan 24, 2024

Conversation

jcscottiii
Copy link
Collaborator

The service will now interpret a query that takes the form of feature:

Similarly to the searchcacheMetadataFetcher, there is now a searchcacheWebFeaturesManifestFetcher

Copy link
Collaborator

@KyleJu KyleJu left a comment

Choose a reason for hiding this comment

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

Note to myself: Need to finish reviews for polling and cache logic

api/query/README.md Outdated Show resolved Hide resolved
api/query/atoms.go Show resolved Hide resolved
api/query/atoms.go Outdated Show resolved Hide resolved
api/query/cache/index/filter.go Outdated Show resolved Hide resolved
api/query/cache/index/filter.go Outdated Show resolved Hide resolved
@jcscottiii jcscottiii force-pushed the jcscottiii/web-features-search-pt2 branch from 32da8d1 to d8bd9d3 Compare January 19, 2024 20:20
@jcscottiii jcscottiii force-pushed the jcscottiii/web-features-search-pt3 branch from 9d67f32 to 42c9201 Compare January 19, 2024 20:27
Base automatically changed from jcscottiii/web-features-search-pt2 to main January 22, 2024 01:27
jcscottiii and others added 2 commits January 22, 2024 01:31
The service will now interpret a query that takes the form of
feature:<web-feature-key>

Similarly to the searchcacheMetadataFetcher, there is now a searchcacheWebFeaturesManifestFetcher
Co-authored-by: Kyle Ju <kyleju@google.com>
@jcscottiii jcscottiii force-pushed the jcscottiii/web-features-search-pt3 branch from 583b4dd to 7d893db Compare January 22, 2024 01:31
@jcscottiii
Copy link
Collaborator Author

This is ready for another review @KyleJu

@KyleJu
Copy link
Collaborator

KyleJu commented Jan 22, 2024

Chatted offline. The downloading Manifest bit can be moved to the shared/ directory

Since most of the logic has been moved into shared, I was able to
unexport a lot of things I thought I would need
@jcscottiii
Copy link
Collaborator Author

Note to reviewers:

Moving all the Github logic to shared allowed me to:

  • unexport a lot of the things
  • add a new test file for the existing poll.go file.

@jcscottiii
Copy link
Collaborator Author

@KyleJu This is ready for another round

Copy link
Collaborator

@KyleJu KyleJu left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for addressing all the comments!

shared/web_features_manifest_github_download.go Outdated Show resolved Hide resolved
api/query/web_features_manifest_cache.go Show resolved Hide resolved
jcscottiii and others added 2 commits January 24, 2024 09:58
Co-authored-by: Kyle Ju <kyleju@google.com>
When testing in my own repo, the Name field worked.
But now that the wpt releases are automated, the label is the correct
field
@jcscottiii
Copy link
Collaborator Author

jcscottiii commented Jan 24, 2024

@KyleJu I added one more commit. I think it should be fine but don't want to assume. Let me know what you think of that commit

Copy link
Collaborator

@KyleJu KyleJu left a comment

Choose a reason for hiding this comment

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

LGTM again

@jcscottiii jcscottiii merged commit ea1c3df into main Jan 24, 2024
12 checks passed
@jcscottiii jcscottiii deleted the jcscottiii/web-features-search-pt3 branch January 24, 2024 19:36
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.

2 participants