Skip to content

feat: configurable norm/case-sensitive/slashes preserving matches - #19

Closed
ankostis wants to merge 2 commits into
miracle2k:masterfrom
ankostis:preserve_matches
Closed

ankostis wants to merge 2 commits into
miracle2k:masterfrom
ankostis:preserve_matches

Conversation

@ankostis

Copy link
Copy Markdown
Contributor

FIX #10 to be possible to preserve the matched captures.

Previously all matches were passed through os.path.normcase(), which in Windows, it converts them to lower-cases.
The resulting matches would then break user-code when it runs on POSIX and non-POSIX systems (e.g. running a Makefile-like python tool).
Note that python stdlib does not by default pass al filepaths through normcase() - this is left to the discretion of the user.

  • Contains also a fix commit( b777c1f) where the lru_cache was not actually used. Was that your intention @miracle2k?
  • Test-cases run OK on Linux python 2 & 3 and CYGWIN python-2 (cannot run in pure Windows python because test-cases are written for forward slash OS).
  • Performance degradation is negligible on the above platforms, as measured by running the test-cases 10 times (I know, this in not a big workload).

…ng matches

+ Performance not seem to be affected when measure with cygwin-python-2.
@miracle2k

Copy link
Copy Markdown
Owner

Looks good, thanks for the patch.

@miracle2k miracle2k closed this May 1, 2017
miracle2k added a commit that referenced this pull request May 1, 2017
…gurable norm/case-sensitive/slashes preserving matches"
@miracle2k

Copy link
Copy Markdown
Owner

The LRU bug was introduced here :( 6876675#diff-b702bab9f1fd8e250b7d429a75486ba2L38

@ankostis

ankostis commented May 1, 2017

Copy link
Copy Markdown
Contributor Author

By the way, to really fix case-conversion of matches(#10), the defaults must change to:

norm_paths=False, case_sensitive='nt' in os.name

@miracle2k

Copy link
Copy Markdown
Owner

You mean there is something else to be done here?

Actually, looking at it more, I think we broke backwards compatibility here. The behaviour changed from not caring about case-sensitivity, to caring about it by default. Am I wrong?

@ankostis

ankostis commented May 7, 2017 •

Copy link
Copy Markdown
Contributor Author

This PR preserves the behavior of the library, but library's behavior so far did not preserve the case of matches.
And this is not what Python's os.path and pathib functions do.

So I said that in order to imitate Python's behavior, you need to set the defaults I described above in the various keyword-arguments.
Such a breaking change would deserve a separate PR/commit.

Does that make sense?

@ankostis

Copy link
Copy Markdown
Contributor Author

You mean there is something else to be done here?

Yes.
Just to be clear before for the next release,
I believe that the defaults for Windows when with_matches=True has to be these:

glob2.glob('stab.*', with_matches=True, case_sensitive=False, norm_paths=False)

and a note that if the program runs also in POSIX, adding sep='/' also helps portability.

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

Successfully merging this pull request may close these issues.

Support case in-sensitive globbing

2 participants