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

Added search using qualifier[:=]value syntax #2373

Merged
merged 16 commits into from
Oct 24, 2023

Conversation

zvonler
Copy link
Contributor

@zvonler zvonler commented Oct 17, 2023

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

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:

  • Architecture
  • Author
  • Category
  • Dependencies
  • License
  • Maintainer
  • Name
  • Paragraph
  • Provides
  • Sentence
  • Types
  • Versions
  • Website

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 following value.

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?

@zvonler zvonler changed the title Added search using qualifier[:=]value syntax #1535 Added search using qualifier[:=]value syntax Oct 17, 2023
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (710ecba) 62.77% compared to head (9fab459) 64.10%.
Report is 3 commits behind head on master.

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     
Flag Coverage Δ
unit 64.10% <100.00%> (+1.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
commands/lib/search.go 96.38% <100.00%> (-0.29%) ⬇️
commands/lib/search_matcher.go 100.00% <100.00%> (ø)
internal/cli/lib/search.go 67.83% <100.00%> (+12.91%) ⬆️

... and 25 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kittaakos kittaakos self-requested a review October 18, 2023 06:47
@alessio-perugini alessio-perugini added this to the Arduino CLI v0.36.0 milestone Oct 18, 2023
@alessio-perugini alessio-perugini added topic: code Related to content of the project itself type: enhancement Proposed improvement labels Oct 18, 2023
@alessio-perugini
Copy link
Contributor

Thank you for the PR! The review will take some time bear with us 🙏

@zvonler
Copy link
Contributor Author

zvonler commented Oct 18, 2023

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.

Copy link
Contributor

@alessio-perugini alessio-perugini left a 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 💪

commands/lib/search.go Outdated Show resolved Hide resolved
commands/lib/search.go Outdated Show resolved Hide resolved
commands/lib/search_test.go Show resolved Hide resolved
commands/lib/search_test.go Outdated Show resolved Hide resolved
@zvonler
Copy link
Contributor Author

zvonler commented Oct 20, 2023

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 task go:test so the integration test was broken initially.

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.

commands/lib/search_matcher.go Outdated Show resolved Hide resolved
Co-authored-by: Alessio Perugini <alessioper.98@gmail.com>
@alessio-perugini
Copy link
Contributor

alessio-perugini commented Oct 23, 2023

@zvonler We're almost there 😃, for the docs it's okay to write them directly in cobra command fields. (internal/cli/lib/search.go)
In the Long property you can explain the new feature: which qualifiers are allowed, the operators (:,=), and how they work.
In the Example property, you can add a few examples showing off the new filters.

@zvonler
Copy link
Contributor Author

zvonler commented Oct 23, 2023

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.

Copy link
Contributor

@alessio-perugini alessio-perugini left a comment

Choose a reason for hiding this comment

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

LGTM. Outstanding work! 🤩

@alessio-perugini alessio-perugini merged commit d8694ec into arduino:master Oct 24, 2023
96 checks passed
@alessio-perugini alessio-perugini linked an issue Dec 5, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow filtering Library Manager search by library name
2 participants