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] Add quotes around the workspace name #37

Merged
merged 4 commits into from
Jul 14, 2015
Merged

[fix] Add quotes around the workspace name #37

merged 4 commits into from
Jul 14, 2015

Conversation

cwill747
Copy link
Contributor

Fixes #36

Adds single quotes around the workspace name to account for possible spaces.

@WtfJoke
Copy link
Member

WtfJoke commented Jul 13, 2015

First of all, thanks again for the fix and not just for the reporting of the issue 😄

To the change itself. I thought it might be better if we quote the workspace already in the configobject (using shell#quote).
Or what do you think about that?

Most likely we hit the same issue, when we have a streamname with space.

And if you agree, would you implement it directly on this pr or should I do it afterwards? ☝️

@WtfJoke WtfJoke changed the title [fix] Add quotes aroudn the workspace name [fix] Add quotes around the workspace name Jul 13, 2015
@cwill747
Copy link
Contributor Author

Duh... That makes way more sense. I will make the change in my PR :)
On Jul 13, 2015 4:14 PM, "Manuel" [email protected] wrote:

First of all, thanks again for the fix and not just for the reporting of
the issue [image: 😄]

To the change itself. I thought it might be better if we quote the
workspace already in the configobject.
Or what do you think about that?

Most likely we hit the same issue, when we have a streamname with space.

And if you agree, would you implement it directly on this pr or should I
do it afterwards? [image: ☝️]


Reply to this email directly or view it on GitHub
#37 (comment).

@WtfJoke
Copy link
Member

WtfJoke commented Jul 13, 2015

Perfect!

@cwill747
Copy link
Contributor Author

How do you feel about changing it to use Python3's string formatting? Would let us do something like

command = "scm --show-alias n --show-uuid y list baselines --components {component} -r {config.repo} -u {config.user} -P {config.password} -m 20000".format(component=entry.component, config=config)

ref: https://docs.python.org/3.4/library/string.html#formatstrings

@cwill747
Copy link
Contributor Author

OK Done, changed in configuration.py instead of rtcFunctions. Tested locally, seems to work ok.

@cwill747
Copy link
Contributor Author

Just kidding, saw what you meant. I used shlex.quote(), I think it might cover some more use cases than your shell file. If you'd rather me use shell.quote, that's fine by me.

@WtfJoke
Copy link
Member

WtfJoke commented Jul 14, 2015

Just kidding, saw what you meant. I used shlex.quote(), I think it might cover some more use cases than your shell file. If you'd rather me use shell.quote, that's fine by me.

Nah, I preferr to have shlex.quote too as long as it works. 😄 For me shlex.quote didnt worked out for the usecase I had (but I forgot the details about it, why it didnt worked)

How do you feel about changing it to use Python3's string formatting?

That looks cool 😋 Tell me if you like to do that or else you can open an issue for that and I will work on that as soon as I can. :)

WtfJoke added a commit that referenced this pull request Jul 14, 2015
[fix] Add quotes around the workspace name (fixes #37)
@WtfJoke WtfJoke merged commit d608dac into rtcTo:develop Jul 14, 2015
@cwill747
Copy link
Contributor Author

Would be happy changing to python3's string formatting, would give me something to do 😄

@cwill747 cwill747 deleted the workspace-whitespace-fix branch July 14, 2015 13:01
@WtfJoke
Copy link
Member

WtfJoke commented Jul 14, 2015

Haha sure, go for it! I appreciate if you can do that 👍

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