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

[AIRFLOW-3174] Refine Docstring for SQL Operators #4023

Closed
wants to merge 0 commits into from

Conversation

XD-DENG
Copy link
Member

@XD-DENG XD-DENG commented Oct 9, 2018

JIRA

https://issues.apache.org/jira/browse/AIRFLOW-3174

Description

  • Here are some details about my PR, including screenshots of any UI changes:

These SQL operators (based on DbApiHook) receive sql and parameters (optional) as arguments. But parameters were not documented.

The contents I added is based on the docstring in https://github.com/apache/incubator-airflow/blob/master/airflow/hooks/dbapi_hook.py#L88

@codecov-io
Copy link

codecov-io commented Oct 9, 2018

Codecov Report

Merging #4023 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4023   +/-   ##
=======================================
  Coverage   75.77%   75.77%           
=======================================
  Files         199      199           
  Lines       15946    15946           
=======================================
  Hits        12083    12083           
  Misses       3863     3863
Impacted Files Coverage Δ
airflow/operators/mssql_operator.py 0% <ø> (ø) ⬆️
airflow/operators/sqlite_operator.py 100% <ø> (ø) ⬆️
airflow/operators/mysql_operator.py 0% <ø> (ø) ⬆️
airflow/operators/oracle_operator.py 0% <ø> (ø) ⬆️
airflow/operators/jdbc_operator.py 0% <ø> (ø) ⬆️
airflow/operators/postgres_operator.py 0% <ø> (ø) ⬆️

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 4e94997...4600b3a. Read the comment docs.

@XD-DENG XD-DENG changed the title [AIRFLOW-XXX] Refine Docstring for SQL Operators [AIRFLOW-3174] Refine Docstring for SQL Operators Oct 9, 2018
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks @XD-DENG for picking this up. Also got some additional comments

@@ -34,6 +34,8 @@ class JdbcOperator(BaseOperator):
:type sql: Can receive a str representing a sql statement,
a list of str (sql statements), or reference to a template file.
Template reference are recognized by str ending in '.sql'
:param parameters: (optional) the parameters to render the SQL query with.
Copy link
Contributor

Choose a reason for hiding this comment

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

autocommit is also missing here :)

@@ -31,6 +31,8 @@ class MsSqlOperator(BaseOperator):
:param sql: the sql code to be executed
:type sql: str or string pointing to a template file with .sql
extension. (templated)
:param parameters: (optional) the parameters to render the SQL query with.
:type parameters: mapping or iterable
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you fix the order? :) The sql one comes before mssql_conn_id, but in the docstring it is the other way around. Also autocommit is missing here.

@@ -32,6 +32,8 @@ class MySqlOperator(BaseOperator):
:type sql: Can receive a str representing a sql statement,
a list of str (sql statements), or reference to a template file.
Template reference are recognized by str ending in '.sql'
:param parameters: (optional) the parameters to render the SQL query with.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you fix the order? :) The sql one comes before mssql_conn_id, but in the docstring it is the other way around. Also autocommit is missing here.

@@ -32,6 +32,8 @@ class OracleOperator(BaseOperator):
:type sql: Can receive a str representing a sql statement,
a list of str (sql statements), or reference to a template file.
Template reference are recognized by str ending in '.sql'
:param parameters: (optional) the parameters to render the SQL query with.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you fix the order? :) The sql one comes before mssql_conn_id, but in the docstring it is the other way around. Also autocommit is missing here.

@@ -31,6 +31,8 @@ class PostgresOperator(BaseOperator):
:type sql: Can receive a str representing a sql statement,
a list of str (sql statements), or reference to a template file.
Template reference are recognized by str ending in '.sql'
:param parameters: (optional) the parameters to render the SQL query with.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you fix the order? :) The sql one comes before mssql_conn_id, but in the docstring it is the other way around. Also autocommit is missing here.

@@ -31,6 +31,8 @@ class SqliteOperator(BaseOperator):
:param sql: the sql code to be executed. (templated)
:type sql: str or string pointing to a template file. File must have
a '.sql' extensions.
:param parameters: (optional) the parameters to render the SQL query with.
:type parameters: mapping or iterable
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you fix the order? :) The sql one comes before mssql_conn_id, but in the docstring it is the other way around.

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.

3 participants