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.

Sign up for GitHub

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

Jump to bottom

Draft: Implement command widgets to textual CLI #8

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Draft: Implement command widgets to textual CLI #8

wants to merge 9 commits into from

Conversation

pavelkraleu
Copy link

@pavelkraleu pavelkraleu commented Jul 23, 2023 •
edited

This PR closes this issue.

textual --help
Usage: textual [OPTIONS] COMMAND [ARGS]...

Options:
  --version  Show the version and exit.
  --help     Show this message and exit.

Commands:
  borders   Explore the border styles available in Textual.
  colors    Explore the design system.
  console   Run the Textual Devtools console.
  diagnose  Print information about the Textual environment.
  easing    Explore the animation easing functions available in Textual.
  keys      Show key events.
  run       Run a Textual app.
  widgets   Explore possible example_widgets.

Example:

textual widgets
example

@pavelkraleu pavelkraleu marked this pull request as ready for review July 23, 2023 14:19
rodrigogiraoserrao
rodrigogiraoserrao reviewed Jul 23, 2023
View reviewed changes
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

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

I'm really excited about this PR.
I've left some comments and questions.

src/textual_dev/previews/widgets.py
Comment on lines +78 to +84
for widget in WIDGETS.keys():
yield Button(widget, id=widget)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could take these two lines of code and write them directly inside WidgetsApp.compose, instead of yield WidgetButtons().
This would also mean you have to adjust the CSS and put it in widgets.css.

src/textual_dev/previews/widgets.py
Comment on lines +93 to +99
for widget_name, widget in WIDGETS.items():
yield from widget(id=widget_name)
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 please use a clearer/more verbose name instead of widget?
Something like widget_composer, or something of the sorts.

src/textual_dev/previews/widgets.py Outdated
Comment on lines 57 to 59
# TabbedContatn missing
# Tabs missing
# TextLog missing
Copy link
Contributor

Choose a reason for hiding this comment

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

There are still some widgets missing, as you note here.
Do you need help with those?
Will you add those in an upcoming commit?

src/textual_dev/previews/example_widgets/__init__.py
id=id,
)

yield Footer()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this footer here? Is it showing any relevant bindings?

@pavelkraleu pavelkraleu changed the title Implement command widgets to textual CLI Draft: Implement command widgets to textual CLI Jul 24, 2023
rodrigogiraoserrao
rodrigogiraoserrao suggested changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

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

Excellent work with the missing widgets.

There's still ListView missing, right? Are you working on it? Do you need help?

.github/workflows/pythonpackage.yml Outdated
Comment on lines 12 to 19
push:
paths:
- '.github/workflows/pythonpackage.yml'
- '**.py'
- '**.pyi'
- '**.css'
- '**.lock'
- 'Makefile'
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change do?

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to run pipeline on each git push to investigate possible problems quickly. This way I can run pipelines in my fork after each commit.
I tried to use YAML anchors to prevent code duplication, but they are not supported on Github so they were removed in later commit.

Copy link
Author

Choose a reason for hiding this comment

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

It seemed to me that ListItem and ListView are basically same examples. So I have skipped one of them.
Am I missing some diference between those two?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right. Sorry.
Maybe the button should say "ListView / ListItem" or something similar.

src/textual_dev/cli.py Show resolved Hide resolved
src/textual_dev/previews/example_widgets/text_log.py Outdated Show resolved Hide resolved
src/textual_dev/previews/example_widgets/__init__.py
Comment on lines +105 to +107
bar = ProgressBar(total=100, show_eta=False)
bar.advance(50)
yield Container(Center(Middle(bar)), id=id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the progress bar is static, which makes sense, maybe create it as ProgressBar() and don't advance it. That way, the user can see the indeterminate animation.

Copy link
Author

Choose a reason for hiding this comment

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

Do I understand it correctly that you want to remove bar.advance(50)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove bar.advance(50) and also total=100.

src/textual_dev/previews/example_widgets/__init__.py Outdated
TabPane,
Markdown,
Tabs,
TextLog,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused import.

Suggested change
TextLog,

Copy link
Author

Choose a reason for hiding this comment

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

After I added remaining widgets, Windows builds started failing. Probably some added widget is not compatible with Windows?

Removing some widgets was an attempt to investigate which one started causing those problems.

Sorry, this Pull request is still WIP 🙈

Copy link
Author

Choose a reason for hiding this comment

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

For me the biggest blockers are those failing Windows tests. Do you know what could be behind that error?
Otherwise I will disable all newly added widgets one by one and see when tests start passing again.

    def test_cli_widgets():
        runner = CliRunner()
        result = runner.invoke(run, ["widgets"])
>       assert result.exit_code == 0
E       AssertionError: assert 1 == 0
E        +  where 1 = <Result AssertionError('Driver must be in application mode')>.exit_code

Copy link
Author

Choose a reason for hiding this comment

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

I have tried disabling new widgets. I have no idea why Windows builds fail, I think it is unrelated to my changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

All widgets work on all platforms. Maybe the test is flakey?

Copy link
Author

Choose a reason for hiding this comment

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

It seems to be constantly failing.
Do you know what could Driver must be in application mode mean?

@pavelkraleu pavelkraleu marked this pull request as draft August 11, 2023 14:49
@pavelkraleu
Copy link
Author

@rodrigogiraoserrao This PR was rushed during the EuroPython sprint. I'm changing it to draft so I can finish it properly.

@pavelkraleu pavelkraleu mentioned this pull request Aug 11, 2023
Closed
@rodrigogiraoserrao
Copy link
Contributor

Feel free to finish the PR as far as the functionality goes, with all the widgets, and remove the test you added because it is not the correct way of testing a Textual app.
Instead, the test will end up in Textual as a snapshot test, as you can see here:

https://github.com/Textualize/textual/blob/dda2cb2be2f1168716b3b16f56406b304b60b708/tests/snapshot_tests/test_snapshots.py#L592-L605

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodrigogiraoserrao rodrigogiraoserrao rodrigogiraoserrao requested changes

@willmcgugan willmcgugan Awaiting requested review from willmcgugan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add command widgets to Textual CLI
2 participants
@pavelkraleu @rodrigogiraoserrao