Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Reuse parseQueryInfo defined in FindUtils for the FindReplace feature. - #12027

Merged
marcelgerber merged 1 commit into
adobe:masterfrom
ficristo:parseQuery-cleanup
Aug 18, 2016
Merged

marcelgerber merged 1 commit into
adobe:masterfrom
ficristo:parseQuery-cleanup

Conversation

@ficristo

Copy link
Copy Markdown
Collaborator

The functions weren't exactly the same so there could be a change in behaviour.
(The flags in regexp were different, and now uses always a regex to search)

Comment thread src/search/FindReplace.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if(findbar) check here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@abose abose modified the milestones: Release 1.6, Release 1.7 Jan 29, 2016
@ficristo ficristo modified the milestones: Release 1.8, Release 1.7 Aug 2, 2016
@marcelgerber

Copy link
Copy Markdown
Contributor

LGTM (I think our FindUtils.parseQueryInfo has served as well for long enough that we can rely on it 😃 ).
Only thing I can bring up is that we don't need the third "caseFold" argument to cm.getSearchCursor any longer (doc: caseFold is only relevant when matching a string. It will cause the search to be case-insensitive.)

I'm also thinking about changing all occurrences of parsedQuery with something along the lines of queryRegexp so it's clear this is always a regexp and we don't need to handle multiple cases. But that's up to you to decide.

@ficristo

Copy link
Copy Markdown
Collaborator Author

I prefer to leave this as is.

@marcelgerber

Copy link
Copy Markdown
Contributor

Fine then.
Merging.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants