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

gensim wrappers single vs double quote issue in windows revsolved #1208

Merged
merged 3 commits into from
Mar 19, 2017

Conversation

prakhar2b
Copy link
Contributor

Fix #671 According to discussion, a review of all process calls in gensim wrappers was needed, to make sure the quotes are correct for Windows too.

@tmylk
Copy link
Contributor

tmylk commented Mar 12, 2017

Are there any other process calls with thiis issue? would be good to fix them all in one PR

@prakhar2b prakhar2b changed the title ldamallet single vs double quote issue in windows resolved [WIP]ldamallet single vs double quote issue in windows resolved Mar 17, 2017
@prakhar2b
Copy link
Contributor Author

prakhar2b commented Mar 19, 2017

Note: As suggested in this comment in #671 , I reviewed all process calls in gensim wrappers, to make sure the quotes are correct for Windows too.

On a side note, the issue was already solved by making changes in the function convert_input according to the code used by the user who opened the issue, but the same changes applied to other functions and in other wrappers too.

More about single vs double quote in windows here

@prakhar2b prakhar2b changed the title [WIP]ldamallet single vs double quote issue in windows resolved ldamallet single vs double quote issue in windows resolved Mar 19, 2017
@prakhar2b prakhar2b changed the title ldamallet single vs double quote issue in windows resolved gensim wrappers single vs double quote issue in windows revsolved Mar 19, 2017
@tmylk tmylk merged commit 0fd63b8 into piskvorky:develop Mar 19, 2017
@tmylk
Copy link
Contributor

tmylk commented Mar 19, 2017

Thanks for the pr

@piskvorky
Copy link
Owner

piskvorky commented Mar 20, 2017

I think you misunderstood the issue. Windows OS needs double quotes in the strings we pass to it, but that has nothing to do with the changes in this PR.

The changes in this PR are a no-op -- they just reformat the "outer quoting" of Python strings, not their content.

@tmylk did you review there are no other single quotes inside strings being passed to Windows, in the wrappers, tutorials etc?

@prakhar2b
Copy link
Contributor Author

prakhar2b commented Mar 20, 2017

@piskvorky ok, I really misunderstood it. Now, I understand the issue.

I grepped through all the files, and I found one instance of such pattern- this line

msg = "Command '%s' return non-zero exit status %d" % (cmd, res)

I can assure there are no other such pattern. Thanks.

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.

3 participants