snowflakedb / snowflake-connector-python

Snowflake Connector for Python
https://pypi.python.org/pypi/snowflake-connector-python/
Apache License 2.0
578 stars 467 forks source link

SNOW-680197: cursor.execute can return None which requires checking #1285

Open tekumara opened 1 year ago

tekumara commented 1 year ago

What is the current behavior?

SnowflakeCursor execute returns SnowflakeCursor | None.

This means every excecute needs to be checked for None. eg:

    c = cur.execute("select * from mytable")

    if c:
        results = c.fetchone()

If the check isn't performed then pyright/vscode will error with

 "fetchone" is not a known member of "None" (reportOptionalMemberAccess)

Or the application will fail at runtime.

What is the desired behavior?

cursor.execute does not return None for any cases, therefore avoiding the need for a None check.

How would this improve snowflake-connector-python?

This would enable the more fluent syntax without the need for a check:

cur.execute("select * from mytable").fetchone()

References, Other Background

From the pyright docs:

reportOptionalMemberAccess [boolean or string, optional]: Generate or suppress diagnostics for an attempt to access a member of a variable with an Optional type. The default value for this setting is 'error'.

sfc-gh-sfan commented 1 year ago

According to the function documentation, None does have legit meaning, so I'm not sure if removing it is the correct behavior.

https://github.com/snowflakedb/snowflake-connector-python/blob/98677d56a6031b8bafb2765a1a2d2976397b0308/src/snowflake/connector/cursor.py#L624-L625

tekumara commented 1 year ago

Yeh None is returned when the caller provides an empty command:

https://github.com/snowflakedb/snowflake-connector-python/blob/98677d56a6031b8bafb2765a1a2d2976397b0308/src/snowflake/connector/cursor.py#L643

Could this be a ValueError exception instead perhaps?

From the python docs:

exception ValueError Raised when an operation or function receives an argument that has the right type but an inappropriate value, and the situation is not described by a more precise exception such as IndexError.

sfc-gh-sfan commented 1 year ago

Could this be a ValueError exception instead perhaps?

This is going to be a behavior change, but it might not be a bad idea. WDYT? @sfc-gh-mkeller

mhasanbulli commented 1 year ago

This would be pretty neat. Is there any update on this @sfc-gh-sfan?