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

[CT-920][CT-1900] Create Click CLI runner and use it to fix dbt docs … #6723

Merged
merged 8 commits into from
Jan 26, 2023

Conversation

aranke
Copy link
Member

@aranke aranke commented Jan 25, 2023

resolves #5544 #6722

Description

Checklist

@aranke aranke requested a review from a team January 25, 2023 12:14
@aranke aranke requested a review from a team as a code owner January 25, 2023 12:14
@cla-bot cla-bot bot added the cla:yes label Jan 25, 2023
Comment on lines +24 to +27
click.echo(f"Serving docs at {port}")
click.echo(f"To access from your browser, navigate to: http://localhost:{port}")
click.echo("\n\n")
click.echo("Press Ctrl+C to exit.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes sense to me. docs serve is CLI-only functionality; it might make more sense to just click.echo, rather than firing a real event.

If we did want these to keep being "real" events / log messages, we could use the Formatting ("FYI") message type that @peterallenwebb added in #6691. I leave it up to your discretion.

In any case, I think we could also just remove the one-off event types that are no longer being used anywhere else in the codebase: ServingDocsPort, ServingDocsAccessInfo, ServingDocsExitInfo

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll keep these click.echo for now, have removed the one-off event types.

tests/cli/profiles.yml Outdated Show resolved Hide resolved
@aranke aranke requested a review from a team as a code owner January 26, 2023 17:14
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

LGTM

tests/cli/test_cli_run.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MichelleArk MichelleArk left a comment

Choose a reason for hiding this comment

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

We should add another command to tox.ini to run tests for tests/cli locally and in CI via tox, or consider moving cli/test_cli_run.py under tests/functional (my preference, since it's using the same underlying framework)

@aranke
Copy link
Member Author

aranke commented Jan 26, 2023

@MichelleArk You beat me to it ;)
I moved the test to tests/functional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants