Bugfix for fnmatch_translate_extended()

Review Request #2351 — Created June 12, 2015 and discarded

Eric Ayers
commons
zundel/extended-translate-fnmatch
380, 381
pants-reviews
dturner-tw, jsirois

Bugfix for fnmatch_translate_extended to match '*' with 0 or more characters.

A pattern like foo*bar wouldn't match the string foobar before

CI is running https://travis-ci.org/twitter/commons/builds/66534681

Updated unit tests.

  • 1
  • 0
  • 1
  • 0
  • 2
Description From Last Updated
Hmm. This adds another char to match at the end. I think that'll mean that a/a* won't match a/a. Is ... Nick Howard (Twitter) Nick Howard (Twitter)
John Sirois
  1. This breaks zsh glob emulation which is the purpose according to the docs.
  2. You're defeating the docs here.
    1. Hmm, I may be defeating the docs, but there was no test for that behavior and it is not the way zsh globbing works as far as I can tell.

      $ zsh
      $ ls check*poms
      checkpoms
      

      I could update the docs, or should just clone this into unpack_jars and do it the way I want?

    2. My experiment concurs, so a doc fix would be excellent.  I did notice that the doc use case may be the cause of the misunderstanding, for *.py, .py is not matched, but that's because it's a dotfile - a tricky corner case.
    3. There are no tests for dot files either. I'm sure they were somewhat broken before this, but my proposed change will make it worse with patterns like '*.java' or *.* that wouldn't have matched a dotfile before. That is going to be a very tricky corner case indeed.

    4. The clone option, although still disturbing to me in a general way, would continue the path of divorcing pantsbuild.pants from twitter.commons.*.  You might lift the whole globs/rglobs/zglobs over to pants/util and then have more freedom to make things work as you wish.
    5. After adding a few more test cases, I couldn't find anything newly broken. I'll consider moving it into pants (and maybe refactoring the tests), but for now, I think a little patch is all we really need.

  3. 
      
David Turner
  1. Ship It!
  2. 
      
Eric Ayers
Eric Ayers
Eric Ayers
John Sirois
  1. 
      
  2. src/python/twitter/common/dirutil/fileset.py (Diff revisions 1 - 4)
     
     
    s/directory/path component/
  3. 
      
Eric Ayers
John Sirois
  1. Ship It!
  2. 
      
Eric Ayers
  1. Thanks for the reviews David & John. I don't have commit access to twitter commons. Let me know if I need to add someone else who does. I'd also like to followup soon with a release of twitter-commons to pypi so we can update the pants dep.

    1. I just restarted your CI which timed out.  Once that goes green I can patch in to master.  I'll note that's complete here.
      As far as the commons sdists publishes, that will be Brian Wickman or ... you'll need to ping tweeps to find out who can do that.
  2. 
      
Nick Howard (Twitter)
  1. 
      
  2. Hmm. This adds another char to match at the end.
    I think that'll mean that a/a* won't match a/a. Is that intended?

    1. No, thanks for pointing that out. I'll add some more test cases and work on it some more.

  3. 
      
Eric Ayers
Review request changed

Status: Discarded

Change Summary:

There is a forthcoming fix for the pants logic that depends on this in the globs handing from the new engine.

Loading...