-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
Added search using qualifier[:=]value syntax #2373
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2373 +/- ##
==========================================
+ Coverage 62.77% 64.10% +1.32%
==========================================
Files 205 207 +2
Lines 19283 19546 +263
==========================================
+ Hits 12104 12529 +425
+ Misses 6119 5923 -196
- Partials 1060 1094 +34
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Thank you for the PR! The review will take some time bear with us 🙏 |
Thanks. I'm still just learning Go, so very interested in the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation side looks good, a bit too functional to my taste. I tried to see if we could shape it differently, but to be honest I didn't come up with something that didn't feel over-engineered.
Would be cool to add some basic integration tests in internal/integrationtest/lib/lib_test.go
, it might highlight some edge cases coming from the cli side. Not super necessarily as we already have unit tests. If you want to skip them it's fine.
Moving all the matcher-related stuff into a new file called search_matcher.go
probably will bring more clarity to the codebase.
Where should this new behavior be documented?
Most likely:
- expanding the description in the cli-side (we take cobra related description and autogenerate the doc).
- and adding a dedicated page
But @per1234 is the best person to answer this 🧙
We're very happy with the quality we're seeing in this PR, you're doing great 💪
Co-authored-by: Alessio Perugini <[email protected]>
b1e1fcb
to
4e04a27
Compare
Sorry for the churn, I did the force push because it was telling me there was a merge conflict with the trunk, and I didn't understand I wasn't running integration tests as part of I think those changes address all the issues raised except documentation. I can take a swing at that if it makes sense, otherwise happy to coordinate with someone else writing the prose. |
Co-authored-by: Alessio Perugini <[email protected]>
@zvonler We're almost there 😃, for the docs it's okay to write them directly in cobra command fields. ( |
Added some docs, and in doing noticed that two index fields weren't searchable yet so I added them to the set. I hope the command documentation is appropriate, I tried to keep it as short as possible but answer the questions users would have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Outstanding work! 🤩
before creating one)
our contributing guidelines
What kind of change does this PR introduce?
More powerful search using a syntax of
qualifier[:=]value
that allows for partial or exact match (still case-insensitive) against specific library metadata fields.The behavior was discussed in #1535
What is the current behavior?
All
lib search
commands compare the provided query terms against a string constructed from several of each library's metadata fields.What is the new behavior?
All queries that do not contain either a ':' or '=' character are handled exactly as before.
Queries that do contain one of the above characters are parsed using new logic that allows for double-quoted strings to contain spaces, and allows for backslash-escaped literal double-quote characters. The resulting tokens are examined for case-insensitive match with the known qualifier names, which are these:
A query token that is one of these names followed by a colon character (':') indicates to search for the following
value
anywhere in the named metadata field. A query token that is one of these names followed by an equals sign ('=') indicates to return only libraries with the named metadata field that matches exactly (case-insensitive) the followingvalue
.Query tokens that do not match either of the two previous tests are handled using the original search algorithm, so that it is still possible to search for, e.g. "sentence" and get the same results as before.
The double-quote handling means one can search for, e.g.
name="Arduino Low Power"
, and the backslash-escaping means one can search for literal double-quotes in specific fields, e.g.sentence:\"
.A query that mixes the
qualifier[:=]value
syntax with non-matching tokens will return only libraries that successfully match all of the tokens, consistent with previous behavior.Does this PR introduce a breaking change, and is titled accordingly?
It does not
Other information
The tests added here use "live" data in the sense that the query the actual library manager index. That approach is used by the existing tests, so I tried to target the same libraries as they do so as not to introduce additional hidden dependency on the index contents, but there is a chance the tests added here will break due to future library manager index changes. I would add more unit tests if there were a framework for populating a mock index to run tests against. I did perform several tests by hand on the command-line of the escaping and quoting.Where should this new behavior be documented?