You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "o-nikolas (via GitHub)" <gi...@apache.org> on 2023/03/07 19:31:59 UTC
[GitHub] [airflow] o-nikolas opened a new pull request, #29962: Separate cli_parser.py into two modules
o-nikolas opened a new pull request, #29962:
URL: https://github.com/apache/airflow/pull/29962
In order to resolve circular imports as well as to help cleanup the ~2k line cli_parser.py module, all CLI config (declaration of Args, Commands, etc) moved to a new `cli_config.py` module. This config module is much larger than the remaining `cli_parser.py` module and contains more of the original code, so it is perhaps more helpful to think of it as the `cli_parser.py` module has been renamed to `cli_config.py` (where most of the code still lives) and then just the methods related to constructing the parser itself have moved to a new `cli_parser.p`y module :)
NOTE: you will need to unhide the diffs for these files since they are quite large
<!--
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] o-nikolas commented on a diff in pull request #29962: Separate cli_parser.py into two modules
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #29962:
URL: https://github.com/apache/airflow/pull/29962#discussion_r1135946312
##########
airflow/cli/cli_config.py:
##########
@@ -0,0 +1,2250 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Command-line interface."""
Review Comment:
Added doc strings (marking as resolved)
--
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] o-nikolas commented on a diff in pull request #29962: Separate cli_parser.py into two modules
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #29962:
URL: https://github.com/apache/airflow/pull/29962#discussion_r1130222797
##########
airflow/cli/cli_config.py:
##########
@@ -0,0 +1,2250 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Command-line interface."""
Review Comment:
> Can we use docstrings of both modules to better describe what they do?
Sure, good suggestion, I'll add those.
>(As a separate issue, calling the modules cli_parser and cli_config seems redundant since they are already in a cli subpackage. It’s probably difficult to rename cli_parser, but we can name the new module config.)
As you said `cli_parser.py` is difficult to change at this point and given that, I think it's cleaner to have both with the same naming format.
--
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 #29962: Separate cli_parser.py into two modules
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29962:
URL: https://github.com/apache/airflow/pull/29962#discussion_r1130482651
##########
airflow/cli/cli_config.py:
##########
@@ -0,0 +1,2250 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Command-line interface."""
Review Comment:
> I think it's cleaner to have both with the same naming format.
We already have `airflow.cli.simple_table` and `airflow.cli.command`, I’d argue _not_ having the `cli_` prefix is the preferred format.
--
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] o-nikolas commented on a diff in pull request #29962: Separate cli_parser.py into two modules
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #29962:
URL: https://github.com/apache/airflow/pull/29962#discussion_r1134577373
##########
airflow/cli/cli_config.py:
##########
@@ -0,0 +1,2250 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Command-line interface."""
Review Comment:
I have also seen issues in the past with module names that are too generic. You could have many `config` modules, and if you happen to need more than one as dependencies in another module you end up renaming them anyway on the import `from airflow.cli import config as cli_config`. I don't think that super likely here, but I generally try to avoid overly generic module names.
--
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] o-nikolas commented on pull request #29962: Separate cli_parser.py into two modules
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29962:
URL: https://github.com/apache/airflow/pull/29962#issuecomment-1468524777
@potiuk @uranusjr: I resolved the conflict with a fresh rebase and made the requested change. This one is ready for review again!
--
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] o-nikolas commented on pull request #29962: Separate cli_parser.py into two modules
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29962:
URL: https://github.com/apache/airflow/pull/29962#issuecomment-1458802449
It will be helpful to checkout the branch to view the "new" `cli_parser.py` module, since the diff doesn't really show you what's left, only what's been moved to `cli_config.py` (which is helpful in its own way of course).
--
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 #29962: Separate cli_parser.py into two modules
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29962:
URL: https://github.com/apache/airflow/pull/29962#discussion_r1128987526
##########
airflow/cli/cli_config.py:
##########
@@ -0,0 +1,2250 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Command-line interface."""
Review Comment:
Can we use docstrings of both modules to better describe what they do?
(As a separate issue, calling the modules `cli_parser` and `cli_config` seems redundant since they are already in a `cli` subpackage. It’s probably difficult to rename `cli_parser`, but we can name the new module `config`.)
--
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 #29962: Separate cli_parser.py into two modules
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29962:
URL: https://github.com/apache/airflow/pull/29962#discussion_r1136603118
##########
airflow/cli/cli_config.py:
##########
@@ -0,0 +1,2258 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""This modules contains the explicit configuration and definition of Airflow
+CLI commands
+"""
Review Comment:
```suggestion
"""Explicit configuration and definition of Airflow CLI commands."""
```
##########
airflow/cli/cli_parser.py:
##########
@@ -16,2251 +16,36 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-"""Command-line interface."""
+"""This module produces and vends a CLI parser object from Airflow CLI
+command configuration (see airflow.cli.cli_config)
+"""
Review Comment:
```suggestion
"""Produce a CLI parser object from Airflow CLI command configuration.
.. seealso:: :mod:`airflow.cli.cli_config`
"""
```
##########
airflow/cli/cli_parser.py:
##########
@@ -16,2251 +16,36 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-"""Command-line interface."""
+"""This module produces and vends a CLI parser object from Airflow CLI
+command configuration (see airflow.cli.cli_config)
+"""
Review Comment:
```suggestion
"""Produce a CLI parser object from Airflow CLI command configuration.
.. seealso:: :mod:`airflow.cli.cli_config`
"""
```
--
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] potiuk merged pull request #29962: Separate cli_parser.py into two modules
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29962:
URL: https://github.com/apache/airflow/pull/29962
--
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] o-nikolas commented on pull request #29962: Separate cli_parser.py into two modules
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #29962:
URL: https://github.com/apache/airflow/pull/29962#issuecomment-1470458258
Used your doc strings @uranusjr, any other feedback?
--
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] potiuk commented on a diff in pull request #29962: Separate cli_parser.py into two modules
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29962:
URL: https://github.com/apache/airflow/pull/29962#discussion_r1135973238
##########
airflow/cli/cli_config.py:
##########
@@ -0,0 +1,2250 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Command-line interface."""
Review Comment:
Yeah. I agree generic module names are a recipe for problems.
--
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