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

Filter requests by folder, URI, method, query string #797

Merged
merged 10 commits into from Mar 28, 2018

Conversation

Projects
None yet
2 participants
@shawnaxsom
Contributor

shawnaxsom commented Mar 5, 2018

Relevant issues?

#739 - This fulfills at least some part of the issue. We may want to update the UI to display either what parts of text are matched, and/or show missing fields (e.g. add URL to the Quick Switch modal).

What does this PR do?

  • Adds a fuzzyMatchAll method to simplify and standardize how multiple-field searches are performed.
  • Applies fuzzyMatchAll to request search bars in the Sidebar and in the Quick Switch modal.
  • Updates those request search bars to have more descriptive placeholders.
  • Adds unit tests for both fuzzyMatch and fuzzyMatchAll.

What is left to do or discuss on the relevant issues?

  • We might want to make Quick Switch window display URI, 2+ folders (all parents), possibly query string. Either multiple rows, and/or show the field that matched. It might be a little much to include many e.g. query string parameters on there, so some things might have to stay hidden, or we have to get creative.
  • Should we keep Fuzzy Matching as implemented? An option to turn it off might be nice. I was wanting to search for POST earlier, and it was matching something like "httP://lOcalhoST". I prefer multiple word non-fuzzy searching for this reason. But I assume others prefer fuzzy matching or it wouldn't be there.
  • If we keep Fuzzy Matching as implemented, consider whether labels are needed. Probably overkill, but we could add optional labels to the search, like "method:POST url:localhost".

Example screenshots, gifs, or videos?

What code can others reuse?

  • fuzzyMatchAll - This calls out to the existing fuzzyMatch misc utility method. The difference is that it takes a string that will be split on spaces for required search terms, and it takes an array of fields that the search terms can match against.

Testing Steps

  • Search for URI, method, query string, folder, and/or request name on Quick Switch window
  • Perform fuzzy search, leaving out some letters for a given field
  • Perform a multiple word search, searching multiple types of fields in the list above
  • Repeat tests for Sidebar search
@welcome

This comment has been minimized.

welcome bot commented Mar 5, 2018

💖 Thanks for opening this pull request! 💖

To help make this a smooth process, please be sure you have first read the
contributing guidelines.

@gschier

This comment has been minimized.

Collaborator

gschier commented Mar 8, 2018

Haven't forgot about this! Should be able to get to it next week 😄

@gschier

Nice work @axs221! Only a few minor comments.

* Appends path of ancestor groups, delimited by forward slashes
* E.g. Folder1/Folder2/Folder3
*/
_groupOf (request) {

This comment has been minimized.

@gschier

gschier Mar 12, 2018

Collaborator

Small thing. This variable would be more accurately described as child or requestOrRequestGroup. Can you rename to be more clear?

This comment has been minimized.

@shawnaxsom

shawnaxsom Mar 19, 2018

Contributor

Sounds good, I'll go with requestOrRequestGroup, verbose but clear as what can be expected

request.url,
request.method,
this._groupOf(request),
...(request.parameters

This comment has been minimized.

@gschier

gschier Mar 12, 2018

Collaborator

It might be more intuitive to join the parameters to the URL? Thoughts?

See

const qs = buildQueryStringFromParams(parameters);
const finalUrl = joinUrlAndQueryString(url, qs);
for an example of URL building.

This comment has been minimized.

@shawnaxsom

shawnaxsom Mar 20, 2018

Contributor

Yeah that makes sense, I will change it as suggested

@gschier

This comment has been minimized.

Collaborator

gschier commented Mar 12, 2018

Responding to your comments in the PR description:

We might want to make Quick Switch window display URI, 2+ folders (all parents), possibly query string. Either multiple rows, and/or show the field that matched. It might be a little much to include many e.g. query string parameters on there, so some things might have to stay hidden, or we have to get creative.

I agree, it's probably best to show as much as possible. Perhaps we could hide most things by default and only show them if they were part of the match?

Should we keep Fuzzy Matching as implemented? An option to turn it off might be nice. I was wanting to search for POST earlier, and it was matching something like "httP://lOcalhoST". I prefer multiple word non-fuzzy searching for this reason. But I assume others prefer fuzzy matching or it wouldn't be there.

I agree that the fuzzy search right now is probably too loose, specifically for the sidebar filter. For the request switcher, the current solution would probably be fine if results were sorted by best match like in Atom/Sublime/etc.

If we keep Fuzzy Matching as implemented, consider whether labels are needed. Probably overkill, but we could add optional labels to the search, like "method:POST url:localhost".

Oh, that's interesting. Like GitHub does with issues. I think labels are a great idea either way because we could add things like status:200, auth:Basic, content-type:json, etc.

Thanks @axs221!

@shawnaxsom shawnaxsom force-pushed the shawnaxsom:feature/fuzzy-match-request-parameters branch from caa89fc to 58e00cc Mar 20, 2018

@shawnaxsom

This comment has been minimized.

Contributor

shawnaxsom commented Mar 20, 2018

@gschier Okay I updated the branch with the minor feedback from the PR, minus the items mentioned in Responding to your comments in the PR description.

Is what is there so far worth merging, and then doing a second PR for the rest? Otherwise I should be able to get to the additional items in the next week or so.

Thank you!

@gschier

This comment has been minimized.

Collaborator

gschier commented Mar 28, 2018

Yep, it looks great!

@gschier gschier merged commit bee9973 into getinsomnia:develop Mar 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gschier

This comment has been minimized.

Collaborator

gschier commented Mar 28, 2018

I was busy for the past couple weeks traveling but will now be more available and responsive to help out if needed 😄

@shawnaxsom

This comment has been minimized.

Contributor

shawnaxsom commented Mar 28, 2018

@gschier Okay thanks! Hope you had a good time traveling; I've been busy too, no worries. I'll put some thought into the UI changes soon and we can discuss.

@gschier

This comment has been minimized.

Collaborator

gschier commented Mar 28, 2018

It was amazing, thanks for asking 😃

Sounds good! Probably best to create a new issue for discussion of the next phase 👍

@gschier

This comment has been minimized.

Collaborator

gschier commented Mar 29, 2018

@axs221 just so you're aware, I'm disabling the new fuzzy matching for URL/parameters until we have some UI to make it more clear what's going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment