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

Side effects of ignored_path regex #17

Closed
fny opened this issue Apr 3, 2012 · 14 comments
Closed

Side effects of ignored_path regex #17

fny opened this issue Apr 3, 2012 · 14 comments

Comments

@fny
Copy link

fny commented Apr 3, 2012

With the current implementation, :ignore takes far more than just "a list of paths (root directory or sub-dir)."

path =~ /#{ignored_path}$/ 

With ignored_path = "happy", you build the regex object /happy$/ using =~ has the unfortunate side effect of matching too many things (e.g. happy, /happy, /unhappy, weird-app-extension.happy.)

Also, when using regular expressions, if I were working in /home/doc and I want to ignore all files that match /do.*$/, listen ignores the working directory entirely since it still matches /home/doc/. Do you strip the basename and parent directories?

I'd recommend making a note of the regex object on the front page along with how the pathnames are built before comparison.

@thibaudgg
Copy link
Member

Please can you sent a pull request with failing specs so we can improve the implementation? Thanks!

@Maher4Ever
Copy link
Contributor

Indeed, a failing spec will help us understand what is the behavior that you would expect.
Maybe the ignore method should accept a regex instead of a string, then ignoring an extension could safely be written as %r{[^/]\.ext$} which would match file.ext and not dir/.ext/.
Also, just to clarify... the root (listened to) directory will not be affected by the ignore regex.

@akerbos
Copy link

akerbos commented Apr 17, 2012

fny got a point; it is not clear from the documentation that the provided strings are matched against path suffixes. Aside from the mentioned issues, even the default ignore values are wrong as the . are interpreted in regexp style; .git would cause paths ending in legit to be dropped. Change here to escape . accordingly. (Also, why is \.svn not in the list?)

But then, doc is even farer away from the code; maybe takine regexp is better.

@fny
Copy link
Author

fny commented Apr 18, 2012

Apologies for not having written the spec yet. I should have the time tomorrow. :)

@Maher4Ever
Copy link
Contributor

@akerbos You are right about escaping the default ignored directories. We'll fix that. Great catch :). About .svn, I've been using only git for the last couple of years to the degree that I totally forgot about svn, that's all to it. We'll add it though.

After @akerbos reported in #21 that changes to files nested within ignored paths gets reported, I found out it is because of the way ignored paths is currently implemented. I think that taking regex in ignore would make the implementation more resilient and allow developers to do whatever they want with it. We could also add a method named ignore_directory for the simple use cases which would take a directory and start matching starting from the base-directory; here is an example for its use:

.
└── sub
    └── sub2
        ├── sub3-1
        ├── sub3-2
        └── sub3-3

5 directories, 0 files
listener = Listen.to(Dir.pwd)
listener.ignore_directory('sub') # ignores 'sub/**'
listener.ignore_directory('sub/sub2/sub3-1') # only ignores 'sub/sub2/sub3-1/**'

@thibaudgg Does this sound good to you?

@akerbos
Copy link

akerbos commented Apr 18, 2012

Sounds good! Please make sure to be very clear what those regexps are supposed to match (total vs relative path, directory/file names vs paths).

Btw, how much effort is it to implement the * resp. ** expansion syntax instead of regexps?

@thibaudgg
Copy link
Member

Yeah, I think having regex only (with some practical examples) will be enough and better that what we have now.

@Maher4Ever
Copy link
Contributor

@akerbos Implementing the shell-paths syntax shouldn't be hard, but it isn't suited IMHO for ruby. I think most ruby programmers would prefer to work with regex instead of that syntax.

@thibaudgg OK. I'll change ignore paths to accept regex for now. The other method could be implemented later on if need be.

@Maher4Ever
Copy link
Contributor

I just re-implemented how ignored paths are handled in Listen. Please look at the specs (starting from here) for now to see what will get ignored and what won't (we'll update the docs when everyone agrees on the current implementations).

@fny Does this fix your problem? Feel free to hack it until you break it :)

@akerbos
Copy link

akerbos commented Apr 19, 2012

Looks good to me. Maybe include specs/tests to specifically check the behaviour of subdirectories of both watched and ignored directories.

@fny
Copy link
Author

fny commented Apr 20, 2012

All other things being equal, with the introduced changes, listener will always compare a pathname against a regular expression object. For as long as those pathnames are truncated uniformly, the specs look fine. ;D

An important point that needs emphasis in the final docs: listener doesn't prefix relative paths with a forward slash:

/start/here/secret.txt    ===> secret.txt
/start/here/more-secrets/ ===> more-secrets/

For example, ^ignored_directory/ will capture the file touched here in the spec...

context 'with an ignored directory' do
  it "doesn't detect the added file" do
    fixtures do |path|
      mkdir 'ignored_directory'

      modified, added, removed = changes(path, :ignore => %r{^ignored_directory/}, :recursive => true) do
        touch 'ignored_directory/new_file.rb'
      end

      added.should be_empty
      modified.should be_empty
      removed.should be_empty
    end
  end
  ...

...but /ignored_directory/ will not.

@Maher4Ever
Copy link
Contributor

@fny @akerbos Thanks to both of you for your great feedback. You can see the tests for watching subdirectories and documentation for the new ignore method added in today's commits. You also made it to the changelog of Listen :)

@thibaudgg
Copy link
Member

Awesome, Thanks!

@akerbos
Copy link

akerbos commented Apr 24, 2012

@Maher4Ever Thank you for your efforts! listen now works beautifully in my application.

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

No branches or pull requests

4 participants