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

Add support of quoted values for SSHConfig #184

Merged
merged 3 commits into from Sep 5, 2014
Merged

Add support of quoted values for SSHConfig #184

merged 3 commits into from Sep 5, 2014

Conversation

@ykalchevskiy
Copy link
Contributor

@ykalchevskiy ykalchevskiy commented Jul 16, 2013

Hi!

Added support of quoted values for SSHConfig (with tests).
Simplified handling of the configuration file: used one universal regexp instead of symbol manipulation and regexp for proxycommand only.

@lndbrg
Copy link
Contributor

@lndbrg lndbrg commented Jan 21, 2014

Can you rebase this on current master and i'll review it?

@ykalchevskiy
Copy link
Contributor Author

@ykalchevskiy ykalchevskiy commented Jan 22, 2014

Done.
BTW this commit adds two features, so they can be separeted. What do you think about it?

@lndbrg
Copy link
Contributor

@lndbrg lndbrg commented Jan 22, 2014

Splitting it would be awesome! Would also be great if you created a separate test for the get_hosts method (I am looking into creating more unittest like tests too and then, we'd at least have a test for that method in place).

@coveralls
Copy link

@coveralls coveralls commented Jan 22, 2014

Coverage Status

Coverage remained the same when pulling 3c8085f on ykalchevskiy:master into 94580ce on paramiko:master.

@ykalchevskiy
Copy link
Contributor Author

@ykalchevskiy ykalchevskiy commented Feb 15, 2014

Ok, I splitted the commit and also moved the get_hosts function into a separate method (with tests).

@lndbrg
Copy link
Contributor

@lndbrg lndbrg commented Feb 15, 2014

Cool!

@ykalchevskiy
Copy link
Contributor Author

@ykalchevskiy ykalchevskiy commented Apr 21, 2014

Hi @lndbrg,
What status does this pull request have? Do you have plans about merging it? I can rebase it on current master, or can move changes into separate branch.

@lndbrg
Copy link
Contributor

@lndbrg lndbrg commented Apr 21, 2014

@ykalchevskiy yes I want this merged, please rebase (again, sorry about that)

It's @bitprophet that does the merging, so we need to pike him. Right now i know that he has quite a long shortlist though.

@ykalchevskiy
Copy link
Contributor Author

@ykalchevskiy ykalchevskiy commented Apr 22, 2014

Thanks @lndbrg, I've rebased it.

Ok, at least we noted him here, may be he will add this to the end of his list :)

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Aug 8, 2014

Months later: I get notified on everything for the repo, no need to namedrop me :D github doesn't make the notification any bigger sadly.

Skimmed the PR and it looks reasonable, so I +1 to the Reviewed label, I'll probably merge this for 1.15 when I finish triage catchup. Thanks!

@bitprophet bitprophet added this to the 1.15 milestone Aug 11, 2014
@bitprophet bitprophet merged commit f258d1e into paramiko:master Sep 5, 2014
1 check passed
1 check passed
@bitprophet
continuous-integration/travis-ci The Travis CI build passed
Details
bitprophet added a commit that referenced this pull request Sep 5, 2014
@ykalchevskiy
Copy link
Contributor Author

@ykalchevskiy ykalchevskiy commented Sep 6, 2014

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants