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

Lay the groundwork for migrating Airflow CLI to Rich+Click #24590

Merged
merged 4 commits into from
Jun 23, 2022

Conversation

blag
Copy link
Contributor

@blag blag commented Jun 21, 2022

This is the first PR of what will be a series of PRs breaking up #22613 into smaller, more reviewable chunks. The end result will be rewriting the existing airflow CLI to use Click instead of argparse. For motivation, please see #22708.

This PR installs Click, adds constraints to Rich_Click so we can rely on some nice features in recent versions of that, adds a new barebones airflow-ng console script, and tweaks some CLI internals to be more flexible between argparse and Click.

To see how this initial groundwork will be used by future PRs, see #22613, and to see how some of this will be used please see #24591.

airflow/cli/__main__.py Outdated Show resolved Hide resolved
airflow/utils/cli.py Outdated Show resolved Hide resolved
@@ -15,3 +15,108 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
import os

import rich_click as click
Copy link
Member

Choose a reason for hiding this comment

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

Since this import is used only locally in this file, I’d prefer it use rich_click.

Suggested change
import rich_click as click
import rich_click

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks like a reasonable suggestion, but that import comes straight out of the rich_click documentation:

Usage

Import as click

To use rich-click, switch out your normal click import with rich-click, using the same namespace:

import rich_click as click

That's it ✨ Then continue to use click as you would normally.

The rich_click package tries to be a superset of Click. When people are trying to add additional options or commands, I am trying to direct them to the Click documentation, since that is what's relevant to them.

If that justification doesn't sway you, I'm happy to change it if you insist (just confirm in a comment below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This got merged before I had a chance to change this.

@kaxil kaxil merged commit afae297 into apache:main Jun 23, 2022
@kaxil kaxil deleted the convert-to-click-cli-pr branch June 23, 2022 16:28
@@ -140,6 +140,7 @@ install_requires =
python-nvd3>=0.15.0
python-slugify>=5.0
rich>=12.4.4
rich-click>=1.3.1
Copy link
Member

Choose a reason for hiding this comment

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

One small thing (for next PR). Rich-click 1.5 introduced some changes that might cause incompatibilities. I tihnk bumping up to > 1.5 is a good idea.

@Bowrna
Copy link
Contributor

Bowrna commented Jul 2, 2022

@blag It's slightly disheartening to see the Click CLI for airflow getting closed after working on it for a few commands. If there is any plan to do the groundwork or research work for checking the best way to set CLI commands for airflow, could you include me too? I am glad to extend my help and give my contribution to that part. Especially with trying something like using mypyc to speed up the code execution, i can help in some research work in that area.

@blag
Copy link
Contributor Author

blag commented Jul 6, 2022

@Bowrna Absolutely, I'll keep you in mind if/when I decide to try refactoring the CLI again. Thank you again for all of your work!

@jedcunningham jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants