-
Notifications
You must be signed in to change notification settings - Fork 54
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
Slash in git execute fix #1575
Slash in git execute fix #1575
Conversation
f281ad4
to
068c600
Compare
068c600
to
ec22970
Compare
Add to release notes |
ec22970
to
ba5e83f
Compare
@@ -27,16 +27,21 @@ | |||
from snowflake.cli.api.identifiers import FQN | |||
from snowflake.connector.cursor import SnowflakeCursor | |||
|
|||
OMIT_FIRST = slice(1, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add comment explaining what is the reasoning behind those slices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason was to replace magic numbers [3:]
, [1:]
.
# Check if path contains quotes and split it accordingly | ||
if '/"' in path and '"/' in path: | ||
if path.count('"') > 2: | ||
raise ValueError('Too much " in path, expected 2.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this more meaningful. Something like "invalid string blah blah..". Also please raise BadUsage error to handle this with better UX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
if path_parts[2] and path_parts[2] != "/": | ||
after_quoted_part = GitManager._split_path_without_empty_parts( | ||
path_parts[2] | ||
) | ||
else: | ||
after_quoted_part = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if path_parts[2] and path_parts[2] != "/": | |
after_quoted_part = GitManager._split_path_without_empty_parts( | |
path_parts[2] | |
) | |
else: | |
after_quoted_part = [] | |
if path_parts[2] == "/": | |
after_quoted_part = [] | |
else: | |
after_quoted_part = GitManager._split_path_without_empty_parts( | |
path_parts[2] | |
) |
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better 👍
ba5e83f
to
3671164
Compare
Pre-review checklist
Changes description
Fixed
git execute
for branches with/
in name.Fixed
execute
command for files with space in name.