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

Fix for windows path formatting (backslash) #30

Merged
merged 2 commits into from
Mar 8, 2017
Merged

Fix for windows path formatting (backslash) #30

merged 2 commits into from
Mar 8, 2017

Conversation

roel0
Copy link
Contributor

@roel0 roel0 commented Mar 8, 2017

No description provided.

@ppannuto ppannuto merged commit aa71874 into ppannuto:master Mar 8, 2017
@ppannuto
Copy link
Owner

ppannuto commented Mar 8, 2017

Looks good to me. That Travis failure has nothing to do with your patch (it's a bit flaky :/ )

I squashed the two commits b/c I don't think the intermediate fellow was terribly valuable.

Thanks for the fix!

@ppannuto
Copy link
Owner

ppannuto commented Mar 8, 2017

Sigh.. just kidding, I didn't look at that Travis carefully enough.

Your approach doesn't work for Python2 (it repr's strings differently). According to the internet, you can use / as a path separator in Windows, so I just changed things to replace \ with /, which should hopefully also make things work for you. Latest master has this change. LMK if it doesn't work for you.

@roel0
Copy link
Contributor Author

roel0 commented Mar 9, 2017

Strange, repr should be just the same between python2 and python3? I'm using python2 anyway:

Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec  5 2015, 20:32:19) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> path = '/tmp/test.csv'
>>> repr(path).replace('\\', r'\\')[1:-1]
'/tmp/test.csv'

Your solution won't work if there are escape sequences in the path:

>>> path="C:\Users\n1\temp.csv"
>>> path.replace("\\", "/")
'C:/Users\n1\temp.csv'
>>> path
'C:\\Users\n1\temp.csv'
>>> print path
C:\Users
1       emp.csv

@ppannuto
Copy link
Owner

ppannuto commented Mar 9, 2017

Interesting... okay, so jumping back to your original approach, this is the failure I see on py2k:

(in export_data2):
 835       # Fix windows path if needed
 836       log.debug(file_path_on_target_machine)
 837       log.debug(repr(file_path_on_target_machine))
 838       file_path_on_target_machine = repr(file_path_on_target_machine).replace("\\", r"\\")[1:-1]
 839       log.debug(file_path_on_target_machine)
 840       log.debug(repr(file_path_on_target_machine))
 841       self._build('EXPORT_DATA2')
 842       self._build(file_path_on_target_machine)

And relevant bits from the output test:

saleae.saleae: DEBUG: /tmp/test.csv
saleae.saleae: DEBUG: u'/tmp/test.csv'
saleae.saleae: DEBUG: '/tmp/test.csv
saleae.saleae: DEBUG: u"'/tmp/test.csv"
...
saleae.saleae: DEBUG: Send >EXPORT_DATA2, '/tmp/test.csv, ALL_CHANNELS, ALL_TIME, CSV, HEADERS, COMMA, HEX, VOLTAGE<

Notice that the file path ends up with unbalanced quotes in the string. I suspect it's an artifact of the [1:-1] slicing that you added:

file_path_on_target_machine = repr(file_path_on_target_machine).replace("\\", r"\\")[1:-1]

but in general it's giving me pause that the repr based approach may be fragile.

Thoughts on another way to handle this?

@roel0
Copy link
Contributor Author

roel0 commented Mar 10, 2017

slicing a repr seems to be a bad idea, I adjusted that part and created a new pull request

ppannuto added a commit that referenced this pull request Feb 9, 2018
 - #30: Fix path formatting, thanks @roel0
 - #34, #35: Improvements to data exporting, thanks @rpolitex
 - #36: Alternate Logic path on Windows, thanks @hexfet
 - #39: Fix spawning of Logic on Linux, thanks @johnthagen
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.

2 participants