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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support search by /s:keyword - symbol, /n:keyword - name #254

Merged
merged 3 commits into from
Oct 29, 2021

Conversation

vuon9
Copy link
Contributor

@vuon9 vuon9 commented Oct 25, 2021

No description provided.

cointop/search.go Outdated Show resolved Hide resolved
@vuon9 vuon9 force-pushed the feature/types-4-search-keyword branch from 9d20ee2 to 6ef43d5 Compare October 26, 2021 08:21
@lyricnz
Copy link
Collaborator

lyricnz commented Oct 26, 2021

Probably don't want to commit your venv ;)

Copy link
Collaborator

@lyricnz lyricnz left a comment

Choose a reason for hiding this comment

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

LGTM

@lyricnz lyricnz marked this pull request as ready for review October 26, 2021 08:35
@lyricnz
Copy link
Collaborator

lyricnz commented Oct 26, 2021

Maybe add a line to the FAQ about this functionality? https://github.com/cointop-sh/cointop/blob/master/docs/content/faq.md#how-do-i-search

@vuon9
Copy link
Contributor Author

vuon9 commented Oct 26, 2021

Maybe add a line to the FAQ about this functionality? https://github.com/cointop-sh/cointop/blob/master/docs/content/faq.md#how-do-i-search

I just updated it!

@lyricnz
Copy link
Collaborator

lyricnz commented Oct 26, 2021

Maybe add a line to the FAQ about this functionality? https://github.com/cointop-sh/cointop/blob/master/docs/content/faq.md#how-do-i-search

I just updated it!

:)

BTW, do you have a specific use-case where needing symbol/name specific matches wouldn't have "just worked" anyway?

@vuon9
Copy link
Contributor Author

vuon9 commented Oct 26, 2021

Maybe add a line to the FAQ about this functionality? https://github.com/cointop-sh/cointop/blob/master/docs/content/faq.md#how-do-i-search

I just updated it!

:)

BTW, do you have a specific use-case where needing symbol/name specific matches wouldn't have "just worked" anyway?

TBH, I think it could be a lot. I think this search function is quite complicated about starting index, and it impacts to the result, I'm not sure so much. If the expectation of result, I think it could be working in another issue if any problem has been figured out. @lyricnz

@lyricnz
Copy link
Collaborator

lyricnz commented Oct 26, 2021

The search behaviour changed recently - now it searches from where the cursor currently is, so you can do repeated search (by hitting / [enter]).

Yes, there is kindof an edge case where if you search for something slightly obscure (say "coin") it first goes to name/symbol=coin, then each repeat starts from that point (with a substring/partial search). It doesn't seem to be possible to do an approximate search before the full-patch result.

@vuon9
Copy link
Contributor Author

vuon9 commented Oct 27, 2021

The search behaviour changed recently - now it searches from where the cursor currently is, so you can do repeated search (by hitting / [enter]).

Yes, there is kindof an edge case where if you search for something slightly obscure (say "coin") it first goes to name/symbol=coin, then each repeat starts from that point (with a substring/partial search). It doesn't seem to be possible to do an approximate search before the full-patch result.

Will it be solved by have temp var for keyword after sanitizing, and keep original keyword storing as current?


Update: The given keyword already properly store as the last query search, I think can ignore my above question.

@lyricnz
Copy link
Collaborator

lyricnz commented Oct 27, 2021

This PR didn't create the issue. It is already present. I'm not sure it's actually a problem though.

cointop/search.go Show resolved Hide resolved
cointop/search.go Show resolved Hide resolved
@miguelmota
Copy link
Member

miguelmota commented Oct 29, 2021

Thanks for the PR @vuon9! LGTM, going to merge 馃憤

@miguelmota miguelmota merged commit 9a9ee30 into cointop-sh:master Oct 29, 2021
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.

None yet

4 participants