You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vincbeck (via GitHub)" <gi...@apache.org> on 2023/05/15 20:03:37 UTC
[GitHub] [airflow] vincbeck opened a new pull request, #31297: Add D400 pydocstyle check - Core Airflow only
vincbeck opened a new pull request, #31297:
URL: https://github.com/apache/airflow/pull/31297
This PR is a follow up of #31135. It had been decided to split #31135 in multiple PRs to make it easier to review. This PR's scope is core Airflow only.
<!--
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 an 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/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #31297: Add D400 pydocstyle check - core Airflow only
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #31297:
URL: https://github.com/apache/airflow/pull/31297#discussion_r1195819059
##########
airflow/models/xcom.py:
##########
@@ -188,7 +191,7 @@ def set(
run_id: str | None = None,
map_index: int = -1,
) -> None:
- """:sphinx-autoapi-skip:"""
+ """:sphinx-autoapi-skip:."""
Review Comment:
Same for everything below
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #31297: Add D400 pydocstyle check - core Airflow only
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #31297:
URL: https://github.com/apache/airflow/pull/31297#discussion_r1195289812
##########
airflow/utils/sqlalchemy.py:
##########
@@ -51,15 +51,14 @@ class UtcDateTime(TypeDecorator):
"""
Almost equivalent to :class:`~sqlalchemy.types.TIMESTAMP` with
``timezone=True`` option, but it differs from that by:
-
- Never silently take naive :class:`~datetime.datetime`, instead it
always raise :exc:`ValueError` unless time zone aware value.
- :class:`~datetime.datetime` value's :attr:`~datetime.datetime.tzinfo`
is always converted to UTC.
- Unlike SQLAlchemy's built-in :class:`~sqlalchemy.types.TIMESTAMP`,
it never return naive :class:`~datetime.datetime`, but time zone
aware value, even with SQLite or MySQL.
- - Always returns TIMESTAMP in UTC
+ - Always returns TIMESTAMP in UTC.
Review Comment:
To be honest I dont understand the previous 2 level lists. To me the new format makes more sense but I must 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on pull request #31297: Add D400 pydocstyle check - Core Airflow only
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #31297:
URL: https://github.com/apache/airflow/pull/31297#issuecomment-1548499859
@uranusjr
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal merged pull request #31297: Add D400 pydocstyle check - core Airflow only
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #31297:
URL: https://github.com/apache/airflow/pull/31297
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #31297: Add D400 pydocstyle check - core Airflow only
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #31297:
URL: https://github.com/apache/airflow/pull/31297#discussion_r1194498655
##########
airflow/models/taskinstance.py:
##########
@@ -2675,7 +2678,7 @@ def ti_selector_condition(cls, vals: Collection[str | tuple[str, int]]) -> Colum
def schedule_downstream_tasks(self, session: Session = NEW_SESSION, max_tis_per_query: int | None = None):
"""
The mini-scheduler for scheduling downstream tasks of this task instance
- :meta: private
+ :meta: private.
"""
Review Comment:
This one should be reformatted
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on pull request #31297: Add D400 pydocstyle check - core Airflow only
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #31297:
URL: https://github.com/apache/airflow/pull/31297#issuecomment-1549844308
> Nice! Just a few things I think would be best addressed in this PR. I have some other quasi-related improvements locally that can be submitted separately after this one is merged.
Thanks for the review! I applied all your suggestion but one :) See my [comment](https://github.com/apache/airflow/pull/31297#discussion_r1195289812)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #31297: Add D400 pydocstyle check - core Airflow only
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #31297:
URL: https://github.com/apache/airflow/pull/31297#discussion_r1194501643
##########
airflow/models/xcom.py:
##########
@@ -172,7 +172,7 @@ def set(
execution_date: datetime.datetime,
session: Session = NEW_SESSION,
) -> None:
- """:sphinx-autoapi-skip:"""
+ """:sphinx-autoapi-skip:."""
Review Comment:
```suggestion
"""Store an XCom value.
:sphinx-autoapi-skip:
"""
```
Similar issue with `:meta private:`. Same for everything below.
##########
airflow/utils/sqlalchemy.py:
##########
@@ -51,15 +51,14 @@ class UtcDateTime(TypeDecorator):
"""
Almost equivalent to :class:`~sqlalchemy.types.TIMESTAMP` with
``timezone=True`` option, but it differs from that by:
-
- Never silently take naive :class:`~datetime.datetime`, instead it
always raise :exc:`ValueError` unless time zone aware value.
- :class:`~datetime.datetime` value's :attr:`~datetime.datetime.tzinfo`
is always converted to UTC.
- Unlike SQLAlchemy's built-in :class:`~sqlalchemy.types.TIMESTAMP`,
it never return naive :class:`~datetime.datetime`, but time zone
aware value, even with SQLite or MySQL.
- - Always returns TIMESTAMP in UTC
+ - Always returns TIMESTAMP in UTC.
Review Comment:
This should probably be reworded a bit, the new format is even more wrong.
##########
airflow/secrets/environment_variables.py:
##########
@@ -35,7 +35,7 @@ def get_conn_uri(self, conn_id: str) -> str | None:
"""
Return URI representation of Connection conn_id.
:param conn_id: the connection id
- :return: deserialized Connection
+ :return: deserialized Connection.
Review Comment:
Should add an empty line instead.
##########
airflow/www/fab_security/manager.py:
##########
@@ -1383,7 +1383,7 @@ def _get_user_permission_resources(
"""
Return a set of resource names with a certain action name
that a user has access to. Mainly used to fetch all menu permissions
- on a single db call, will also check public permissions and builtin roles
+ on a single db call, will also check public permissions and builtin roles.
Review Comment:
Should add a newline after ``access to.`` instead.
##########
airflow/utils/file.py:
##########
@@ -267,7 +267,7 @@ def find_path_from_directory(
Recursively search the base path and return the list of file paths that should not be ignored.
:param base_dir_path: the base path to be searched
:param ignore_file_name: the file name in which specifies the patterns of files/dirs to be ignored
- :param ignore_file_syntax: the syntax of patterns in the ignore file: regexp or glob
+ :param ignore_file_syntax: the syntax of patterns in the ignore file: regexp or glob.
Review Comment:
Should add empty line instead
##########
airflow/www/api/experimental/endpoints.py:
##########
@@ -157,7 +157,7 @@ def delete_dag(dag_id):
def dag_runs(dag_id):
"""
Returns a list of Dag Runs for a specific DAG ID.
- :query param state: a query string parameter '?state=queued|running|success...'
+ :query param state: a query string parameter '?state=queued|running|success...'.
Review Comment:
Wrong here
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -558,18 +566,19 @@ def security_cleanup(self):
You can use it always or just sometimes to
perform a security cleanup. Warning this will delete any permission
that is no longer part of any registered view or menu.
- Remember invoke ONLY AFTER YOU HAVE REGISTERED ALL VIEWS
+ Remember invoke ONLY AFTER YOU HAVE REGISTERED ALL VIEWS.
"""
self.sm.security_cleanup(self.baseviews, self.menu)
def security_converge(self, dry=False) -> dict:
"""
- This method is useful when you use:
- - `class_permission_name`
- - `previous_class_permission_name`
- - `method_permission_name`
- - `previous_method_permission_name`
- migrates all permissions to the new names on all the Roles
+ This method is useful when you use:
+ - `class_permission_name`
+ - `previous_class_permission_name`
+ - `method_permission_name`
+ - `previous_method_permission_name`
+ Migrates all permissions to the new names on all the Roles.
Review Comment:
```suggestion
Migrates all permissions to the new names on all the Roles.
This method is useful when you use:
- `class_permission_name`
- `previous_class_permission_name`
- `method_permission_name`
- `previous_method_permission_name`
```
##########
airflow/www/fab_security/manager.py:
##########
@@ -588,14 +588,14 @@ def get_oauth_token_secret_name(self, provider):
"""
Returns the token_secret name for the oauth provider
if none is configured defaults to oauth_secret
- this is configured using OAUTH_PROVIDERS and token_secret
+ this is configured using OAUTH_PROVIDERS and token_secret.
Review Comment:
I think the period should be put behind ``oauth_secret`` above.
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -62,7 +62,7 @@ def __init__(self, appbuilder):
"""
Class constructor
param appbuilder:
- F.A.B AppBuilder main object
+ F.A.B AppBuilder main object.
Review Comment:
Should add a period ane newline after ``Class constructor``.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org