python / cpython

The Python programming language
https://www.python.org
Other
63.31k stars 30.31k forks source link

Fix for bug 494589 #37648

Closed daf46b87-a9e9-4381-bf23-8c373c9135e6 closed 17 years ago

daf46b87-a9e9-4381-bf23-8c373c9135e6 commented 21 years ago
BPO 658599
Nosy @tim-one, @mhammond, @tebeka, @jackdied
Files
  • ntpath.diff: Diff file
  • nt2.diff: patch 2 (nn)
  • expandvars.patch: Patch with test suite
  • ntpath.diff: diff from previous fix
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = 'https://github.com/mhammond' closed_at = created_at = labels = ['invalid', 'library'] title = 'Fix for bug 494589' updated_at = user = 'https://github.com/tebeka' ``` bugs.python.org fields: ```python activity = actor = 'nnorwitz' assignee = 'mhammond' closed = True closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'tebeka' dependencies = [] files = ['4826', '4827', '4828', '4829'] hgrepos = [] issue_num = 658599 keywords = ['patch'] message_count = 14.0 messages = ['42102', '42103', '42104', '42105', '42106', '42107', '42108', '42109', '42110', '42111', '42112', '42113', '42114', '42115'] nosy_count = 5.0 nosy_names = ['tim.peters', 'mhammond', 'nnorwitz', 'tebeka', 'jackdied'] pr_nums = [] priority = 'normal' resolution = 'not a bug' stage = None status = 'closed' superseder = None type = None url = 'https://bugs.python.org/issue658599' versions = ['Python 2.2'] ```

    daf46b87-a9e9-4381-bf23-8c373c9135e6 commented 21 years ago

    This is a fix for bug 494589 (os.path.expandvars) I suggest using the same code in ntpath and posixpath. (Maybe have a commonpath.py and let both import it?)

    Python version 2.2.2 OS: NT4 SP6 (checked on NT and cygwin)

    Miki

    d21744ff-f396-4c71-955e-7dbd2e886779 commented 21 years ago

    Logged In: YES user_id=33168

    There's no uploaded file! You have to check the checkbox labeled "Check to Upload & Attach File" when you upload a file.

    Please try again.

    (This is a SourceForge annoyance that we can do nothing about. :-( )

    daf46b87-a9e9-4381-bf23-8c373c9135e6 commented 21 years ago

    Logged In: YES user_id=358087

    This time the checkbox is checked. :-)

    Miki

    d21744ff-f396-4c71-955e-7dbd2e886779 commented 21 years ago

    Logged In: YES user_id=33168

    The patch didn't apply for me, so I created a new one and attached it. I can't test this. Maybe Tim is interested.

    tim-one commented 21 years ago

    Logged In: YES user_id=31435

    Mark, can you make time to look at this? I can't.

    mhammond commented 21 years ago

    Logged In: YES user_id=14198

    Is there any reason why:

    from posixpath import expandvars

    is not a better patch? From what I can see, posixpath's version works fine for Windows (windows os.environ is case insensitive)

    mhammond commented 21 years ago

    Logged In: YES user_id=14198

    In fact, why not go the whole-hog, and remove all code in ntpath.py that is identical to posixpath.py

    Example patch attached \<wink>

    tim-one commented 21 years ago

    Logged In: YES user_id=31435

    Sounds like an excellent idea to me, Mark! The glory is all yours, if you're man enough to accept it \<wink>.

    mhammond commented 21 years ago

    Logged In: YES user_id=14198

    It was late last night - the idea of ripping out all duplicated code wont work. A consolidation may be possible, but I haven't time. I'm deleting that patch, but still believe that

    from posixpath import expandvars

    is reasonable. Comments?

    tim-one commented 21 years ago

    Logged In: YES user_id=31435

    Did you look at bug 494589? As I noted there, there are semantic diffferences between the ntpath and posixpath versions of .expandvars() (like ntpath mapping $$ to $, and not expanding within single quotes).

    I personally have no use for the differences, but can't say whether anyone else does. The author of the ntpath version took time to write comments about its pecularities, so they weren't accidents at the time. Incompatible changes are usually PEP material.

    mhammond commented 21 years ago

    Logged In: YES user_id=14198

    Yes, I was too eager there.

    So back to the original patch - it looks good, except it seems to fail in one case I can see: $FOO$FOO

    is not correctly expanded. Put a space between the vars, or enclose them in braces, and it works correctly.

    This isn't really a regression though - the old code doesn't handle that case correctly either. posixpath does.

    See the new patch I uploaded - it contains the original code, plus a patch to test_ntpath.py to test the semantics. Is it possible to fix the patch to handle this case? I haven't time to dig out my regex book \<wink>

    daf46b87-a9e9-4381-bf23-8c373c9135e6 commented 21 years ago

    Logged In: YES user_id=358087

    Hopefully this should do the trick (if I'll remember to attache the file :-) All I did was to allow a $ only right after the first one.

    Miki

    jackdied commented 17 years ago

    does the following svn message mean this should be closed?

    r53460 | sjoerd.mullender | 2007-01-16 11:42:38 -0500 (Tue, 16 Jan 2007) | 4 lines

    Fixed ntpath.expandvars to not replace references to non-existing variables with nothing. Also added tests. This fixes bug bpo-494589.

    d21744ff-f396-4c71-955e-7dbd2e886779 commented 17 years ago

    Given the bug is closed, seems reasonable to close this patch too. Thanks for pointing it out Jack.