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

Show similar library names in lib search #598

Merged
merged 4 commits into from
Apr 2, 2020

Conversation

2qar
Copy link
Contributor

@2qar 2qar commented Feb 27, 2020

Closes #107

Small typos when searching for libraries will show similar library names instead of showing no results.

Emphasis on "small", though, because the similarity value check is hard-coded at a value that's a little strict (maybe this could be a config option?).

@rsora
Copy link
Contributor

rsora commented Mar 26, 2020

Hi @BigHeadGeorge thanks for you contribution!
Sorry for the delayed review 😸 Can you please rebase your PR, so we can review it again and merge it at the speed of light?
Thanks!

@rsora
Copy link
Contributor

rsora commented Mar 31, 2020

Hi @BigHeadGeorge It seems that your last commit passes all the tests, but the commit history has some problems, can you try a rebase again?

Maybe a git rebase --interactive could help in this case.

Thanks again for contributing!

@2qar
Copy link
Contributor Author

2qar commented Mar 31, 2020

Sorry for the messy commit history! This is my first time rebasing. I'll get it cleaned up real quick.

Copy link
Contributor

@rsora rsora left a comment

Choose a reason for hiding this comment

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

Nice job with the rebase 😉 thanks!
I want to ask you if you could be so nice to add a unit test for the new logic you added.

It could be something similar to what we did for the debug command (https://github.com/arduino/arduino-cli/blob/master/commands/debug/debug_test.go). You could proceed like this:

  1. Extracting a function searchLibrary(req *rpc.LibrarySearchReq ,lm *librariesmanager.LibrariesManager) containing all the search code, and placing it in the same file (commands/lib/search.go).

  2. Create a test function in commands/lib/search_test.go that use the searchLibrary function defined above, passing a custom LibrariesManager built using:

Here an example of the test structure, I hope it helps a bit:

// file commands/lib/search_test.go
// ..imports..
// This test expects to find a `commands/lib/testdata` folder containing a fake librery_index.json
var customIndexPath = paths.New("testdata", "custom_lib_index")

func TestSearchLibrary(t *testing.T) {
	lm := librariesmanager.NewLibraryManager(customIndexPath, nil)
	lm.LoadIndex()

	req := &rpc.LibrarySearchReq{
		Instance:   &rpc.Instance{Id: 1},
		Query:    (strings.Join(args, "--your test query-- ")),
	}

	resp, err := searchLibrary(req, lm)
        // ...your asserts on the `resp` object...

Thanks again for your time!

if len(res) == 0 {
status = rpc.LibrarySearchStatus_failed
for _, lib := range lm.Index.Libraries {
if godice.CompareString(req.GetQuery(), lib.Name) > 0.7 {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this 0.7 as package variable (see what we did here for example https://github.com/arduino/arduino-cli/blob/master/telemetry/telemetry.go#L31).
I think keeping it as a variable is sufficient, as I'm afraid that having it in the configuration file could create confusion.

@rsora rsora added this to the 0.10.0 milestone Apr 1, 2020
2qar added 3 commits April 1, 2020 10:54
Just check that the library names have "Test" in them instead of
checking the names at each index, which won't always be the same
Copy link
Contributor

@rsora rsora left a comment

Choose a reason for hiding this comment

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

This is High Quality Contributing ™️ !

FYI, I had to re-run tests for your last commit, because we have a flaky integration test in the telemetry part that we need to take care of.

Excellent work with the testing part!

@rsora rsora merged commit 38a80a6 into arduino:master Apr 2, 2020
@2qar 2qar deleted the lib-search-show-similar branch April 2, 2020 15:11
@2qar
Copy link
Contributor Author

2qar commented Apr 2, 2020

Thanks for the help throughout the process (especially the rebase, kinda panicked when I saw 20 unrelated commits in my PR)

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.

arduino-cli lib search should display nearest names
2 participants