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

Fixed broken functionalities when running on Python2 #263

Merged
merged 1 commit into from
Aug 10, 2019

Conversation

geleems
Copy link

@geleems geleems commented Aug 8, 2019

Some code is broken when running on Python2.

  • str type vs unicode type
    • string is unicode type in Python3 is , but str type in Python2
    • In many places, the updated prompt-toolkit has assertion to check if string param is in unicode
  • User-input string from command prompt (such as db password) contains \r character at the end, which leads to invalid credential failure when connecting to db.

@geleems geleems requested a review from pensivebrian August 8, 2019 19:34
@geleems geleems self-assigned this Aug 8, 2019
return sql
else:
sql = sql.replace('\r', ' ').replace('\n', ' ').strip()
sql = re.sub(r' +', ' ', sql)
Copy link
Member

@pensivebrian pensivebrian Aug 8, 2019

Choose a reason for hiding this comment

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

we didn't need these replace and substitutions before. Why now?

Copy link
Author

@geleems geleems Aug 8, 2019

Choose a reason for hiding this comment

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

I noticed some of our code recognized the type of command by looking at a word before the first space character. For example:

# mssqlcli\packages\special\main.py

def parse_special_command(sql):

    # split the sql by space -- 'command' is a word before first space.
    command, _, arg = sql.partition(' ')

    verbose = '+' in command
    command = command.strip().replace('+', '')
    return (command, verbose, arg.strip())

This assumes the word before the first space character is keyword of command (very fragile). Unfortunately we have methods that returns sql with bunch of leading white spaces. For example:

def get_sql():
    return '''
        SELECT col1,
               col2
        FROM t1
        '''

This method returns a string:

'\n        SELECT col1,\n               col2\n        FROM t1\n        '

The word before the first space is \n that is recognized as a command keyword. This not intended. And the leading \n prevents to strip all the white spaces in front of actual keyword. For example, sql.strip().startswith('SELECT') is false. And this way of peeking keyword is actually exists in our code.

It seems this behavior did not break existing functionality, and I guess it was because that type of parsing was only done for special command which starts with \\. (Neither \n or SELECT is special command so it has returned false anyway and did not break functionality -- working code relying on a bug.)

However, I still believe we should return a sql with correct format since there is an evidence authors assume the first word is a keyword for command.

The normalize() method I wrote converts the raw format into correct format like:

'SELECT col1, col2 FROM t1'



def get_views():
"""
Query string to retrieve all views.
:return: string
"""
return '''
sql = '''
Copy link
Member

Choose a reason for hiding this comment

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

We can't add a u string prefix and assume unicode?

Copy link
Author

Choose a reason for hiding this comment

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

I can put u in front of ''' for making unicode. But we still need normalize() method for removing unintended spaces.

mssqlcli/main.py Show resolved Hide resolved
@geleems geleems requested a review from pensivebrian August 8, 2019 22:01
@geleems geleems merged commit ec51c2c into master Aug 10, 2019
@geleems geleems deleted the gelee/updatefix branch August 14, 2019 19:08
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