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

Quote arguments in aliases #2689

Merged
merged 1 commit into from
Jul 7, 2017
Merged

Conversation

JordonPhillips
Copy link
Member

@JordonPhillips JordonPhillips commented Jun 29, 2017

This quotes arguments passed in to aliases.

I've run the tests for this on Windows in the following shells: git bash, powershell, cmd, and cygwin.

Fixes #2653
Also fixes #2657 (comment)

cc @jamesls @dstufft @kyleknap @stealthycoin

@JordonPhillips JordonPhillips added the pr:needs-review This PR needs a review from a Member. label Jun 29, 2017
@codecov-io
Copy link

codecov-io commented Jun 30, 2017

Codecov Report

Merging #2689 into develop will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2689      +/-   ##
===========================================
+ Coverage    95.56%   95.57%   +0.01%     
===========================================
  Files          151      151              
  Lines        11801    11831      +30     
===========================================
+ Hits         11278    11308      +30     
  Misses         523      523
Impacted Files Coverage Δ
awscli/compat.py 77.35% <100%> (+8.52%) ⬆️
awscli/alias.py 99.06% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2509b8f...6c03ead. Read the comment docs.

buff = []
num_backspaces = 0
for character in s:
if character == '\\':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny nitpick (that you can feel free to ignore), but I think I would prefer this to be r'\' (here and throughout) if only because it doesn't require mentally processing the escape to parse what this code is doing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree, but unfortunately r'\' isn't a valid string since the backslash can still escape quotes in raw strings. Really frustrating because it makes the tests hard to read.

This quotes arguments passed in to aliases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:needs-review This PR needs a review from a Member.
Projects
None yet
4 participants