Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Add 'Find in Open Files' feature #967

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trevdor
Copy link

@trevdor trevdor commented Oct 24, 2017

For Review Only

Suggestions extremely welcome!

Description of the Change

Uses PathSearcher/PathReplacer from scandal to implement find/replace in open files.

Alternate Designs

Tried using Project Find and using the pathsEditor to scope the search to currently open files. It was kinda wonky, and @lee-dohm suggested PathSearcher and a separate panel as an alternative.

Benefits

  • Adds a feature people have been requesting

Possible Drawbacks

  • Duplicates a lot of code from project-find-view, which in turn duplicates a lot from find-view.
    • Could the common code be extracted? I don't personally feel adequately familiar with the project to do this confidently.

Applicable Issues

#241 & #354

TODOs

  • Fix 27/57 failing tests in my new open-files-find-view-spec.js
    • Best I can tell, the behaviors under test work as expected. (maybe a few exceptions)
    • 🆘 Something borked with my test setup?
  • Add tests for results-model#replacePaths
  • Remove/update tests copied from project-find-view-spec that aren't applicable or need different setup

@rubo77
Copy link

rubo77 commented Jun 13, 2020

Can you explain how someone can try your code and start programming on the atom project to help developing a test for this?

@rubo77
Copy link

rubo77 commented Jun 14, 2020

@darangi could you please add the needed tests, so this can be merged?

@darangi
Copy link
Contributor

darangi commented Jun 15, 2020

Can you explain how someone can try your code and start programming on the atom project to help developing a test for this?

Hi @rubo77, the guide on the atom flight manual on how to contribute to official atom packages is a step in the right direction. Do check it out 🙂

@rubo77
Copy link

rubo77 commented Jun 15, 2020

@darangi I think the PR author here has a problem to add the right test, although his PR works already if you compile atom with it.

It would be great, if you would find the time to write a fast test for this PR so you can merge it, since it is one of the most-wanted feature in Atom ;)

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