You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/04/23 22:08:00 UTC
[GitHub] [airflow] Goodkat opened a new pull request #15510: OdbcHook returns None. Related to #15016 issue.
Goodkat opened a new pull request #15510:
URL: https://github.com/apache/airflow/pull/15510
This PR is related to #15016 issue.
OdbcHook returns None for non-boolean-like string values in connect_kwargs dict arg, however connect_kwarg values should remain as is in this case.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] marcosmarxm commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
marcosmarxm commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-826403216
@Goodkat can you check discussion on #15016? Maybe is possible to remove this function and change documentation to use JSON object with true/false values. If is possible removing the function will turn the code cleaner =)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] marcosmarxm commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
marcosmarxm commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-826084662
Nice @Goodkat! Can you add a simple test for this? https://github.com/apache/airflow/blob/master/tests/providers/odbc/hooks/test_odbc.py already has tests for other connections functionalities and add this will helpfully
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Goodkat commented on a change in pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
Goodkat commented on a change in pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#discussion_r620140939
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -171,6 +171,8 @@ def clean_bool(val): # pylint: disable=inconsistent-return-statements
return True
elif val.lower() == 'false':
return False
+ else:
+ return val
Review comment:
As the clean_bool is planned to be removed, the pylint comment will be cleaned as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Goodkat edited a comment on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
Goodkat edited a comment on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-860204343
@potiuk I would better use true/false without capital letters:
```json
"connect_kwargs": {
"autocommit": false,
"ansi": true
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Goodkat commented on a change in pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
Goodkat commented on a change in pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#discussion_r619654732
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -171,6 +171,8 @@ def clean_bool(val): # pylint: disable=inconsistent-return-statements
return True
elif val.lower() == 'false':
return False
+ else:
+ return val
Review comment:
There are different types (at least boolean and string) that could be returned by this function. Therefore (I suppose) the code analyser complains of this fact.
However, as I realized, it could be acceptable sometimes in dynamic languages, so we can conditionally return different data types.
In our case the clean_bool() function's result is used as a value in the dictionary and dictionaries itself can contain the keys/values of the different types. Therefore we may ignore this warning and remove the pylint comment from L168.
Please tell me if it does make sense for you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#discussion_r619606736
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -171,6 +171,8 @@ def clean_bool(val): # pylint: disable=inconsistent-return-statements
return True
elif val.lower() == 'false':
return False
+ else:
+ return val
Review comment:
With this change we may be able to remove the pylint comment from L168, WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-860197757
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Goodkat commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
Goodkat commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-860191624
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Goodkat commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
Goodkat commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-836757441
@eladkal should be OK now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #15510:
URL: https://github.com/apache/airflow/pull/15510
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#discussion_r619606736
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -171,6 +171,8 @@ def clean_bool(val): # pylint: disable=inconsistent-return-statements
return True
elif val.lower() == 'false':
return False
+ else:
+ return val
Review comment:
With this change we may be able to remove the pylint comment from L168, WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-836320972
@Goodkat can you fix the failing test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-860197968
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-860197757
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-860176329
@Goodkat are you still working on it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-838054629
> And it think we _could_ optionally first fix the `if` statement by adding an `else return val` in the current provider major >version, before removing `clean_bool` in next
I agree. If we can preserve backward compatibility with deprecation notice we should. we can remove `clean_bool` in the in a followup major release (whenever it may be)
In any case the steps to bump the major version of provider are:
1. bump [provider yaml](https://github.com/apache/airflow/blob/master/airflow/providers/odbc/provider.yaml#L24)
2. add detailed explanation about the change in the [provider changelog](https://github.com/apache/airflow/blob/master/airflow/providers/odbc/CHANGELOG.rst)
@potiuk did I miss something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Goodkat commented on a change in pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
Goodkat commented on a change in pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#discussion_r619654732
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -171,6 +171,8 @@ def clean_bool(val): # pylint: disable=inconsistent-return-statements
return True
elif val.lower() == 'false':
return False
+ else:
+ return val
Review comment:
There are different types (at least boolean and string) that could be returned by this function. Therefore (I suppose) the code analyser complains of this fact.
However, as I realized, it could be acceptable sometimes in dynamic languages, so we can conditionally return different data types.
In our case the clean_bool() function's result is used as a value in the dictionary and dictionaries itself can contain the keys/values of the different types. Therefore we may ignore this warning and remove the pylint comment from L168.
Please tell me if it does make sense for you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] marcosmarxm commented on pull request #15510: OdbcHook returns None. Related to #15016 issue.
Posted by GitBox <gi...@apache.org>.
marcosmarxm commented on pull request #15510:
URL: https://github.com/apache/airflow/pull/15510#issuecomment-826084662
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org