Draft: Implement command widgets to textual CLI #8

pavelkraleu · 2023-07-23T14:05:19Z

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

rodrigogiraoserrao

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

rodrigogiraoserrao · 2023-07-23T14:33:15Z

src/textual_dev/previews/widgets.py

+        for widget in WIDGETS.keys():
+            yield Button(widget, id=widget)


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.

rodrigogiraoserrao · 2023-07-23T14:33:58Z

src/textual_dev/previews/widgets.py

+            for widget_name, widget in WIDGETS.items():
+                yield from widget(id=widget_name)


Could you please use a clearer/more verbose name instead of widget?
Something like widget_composer, or something of the sorts.

rodrigogiraoserrao · 2023-07-23T14:34:50Z

src/textual_dev/previews/widgets.py

+    # TabbedContatn missing
+    # Tabs missing
+    # TextLog missing


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

rodrigogiraoserrao · 2023-07-23T14:35:58Z

src/textual_dev/previews/example_widgets/__init__.py

+        id=id,
+    )
+
+    yield Footer()


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

rodrigogiraoserrao

Excellent work with the missing widgets.

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

rodrigogiraoserrao · 2023-07-26T09:44:15Z

.github/workflows/pythonpackage.yml

+  push:
+    paths:
+      - '.github/workflows/pythonpackage.yml'
+      - '**.py'
+      - '**.pyi'
+      - '**.css'
+      - '**.lock'
+      - 'Makefile'


What does this change do?

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.

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?

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

src/textual_dev/cli.py

src/textual_dev/previews/example_widgets/text_log.py

rodrigogiraoserrao · 2023-07-26T10:06:37Z

src/textual_dev/previews/example_widgets/__init__.py

+    bar = ProgressBar(total=100, show_eta=False)
+    bar.advance(50)
+    yield Container(Center(Middle(bar)), id=id)


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.

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

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

rodrigogiraoserrao · 2023-07-26T10:07:04Z

src/textual_dev/previews/example_widgets/__init__.py

+    TabPane,
+    Markdown,
+    Tabs,
+    TextLog,


Remove unused import.

Suggested change

TextLog,

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 🙈

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

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

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

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

pavelkraleu · 2023-08-11T14:51:04Z

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

rodrigogiraoserrao · 2023-08-17T10:10:35Z

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

pavelkraleu force-pushed the command-widgets branch from 7919ee0 to f4752f7 Compare July 23, 2023 14:11

pavelkraleu marked this pull request as ready for review July 23, 2023 14:19

rodrigogiraoserrao requested a review from willmcgugan July 23, 2023 14:27

rodrigogiraoserrao reviewed Jul 23, 2023

View reviewed changes

pavelkraleu changed the title ~~Implement command widgets to textual CLI~~ Draft: Implement command widgets to textual CLI Jul 24, 2023

rodrigogiraoserrao suggested changes Jul 26, 2023

View reviewed changes

pavelkraleu marked this pull request as draft August 11, 2023 14:49

pavelkraleu added 3 commits August 11, 2023 16:54

Add .idea/ to .gitignore

67d139f

Remove dependency for Python 3.7 from pre-commit

a6b4280

Test

bb57f01

pavelkraleu force-pushed the command-widgets branch from 9b4b9f4 to bb57f01 Compare August 11, 2023 15:04

pavelkraleu added 6 commits August 11, 2023 17:11

Test

f72c635

Test

79f739f

Test

e87cc09

Test

c00a1e3

Test

1b650d1

Test

d9d7315

pavelkraleu mentioned this pull request Aug 11, 2023

Example showing that builds on Windows fail #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Implement command widgets to textual CLI #8

Draft: Implement command widgets to textual CLI #8

pavelkraleu commented Jul 23, 2023 •

edited

Loading

rodrigogiraoserrao left a comment

rodrigogiraoserrao Jul 23, 2023

rodrigogiraoserrao Jul 23, 2023

rodrigogiraoserrao Jul 23, 2023

rodrigogiraoserrao Jul 23, 2023

rodrigogiraoserrao left a comment

rodrigogiraoserrao Jul 26, 2023

pavelkraleu Jul 27, 2023

pavelkraleu Jul 27, 2023

rodrigogiraoserrao Jul 28, 2023

rodrigogiraoserrao Jul 26, 2023

pavelkraleu Jul 27, 2023

rodrigogiraoserrao Jul 28, 2023

rodrigogiraoserrao Jul 26, 2023

pavelkraleu Jul 27, 2023

pavelkraleu Jul 27, 2023

pavelkraleu Jul 27, 2023

rodrigogiraoserrao Jul 28, 2023

pavelkraleu Jul 28, 2023

pavelkraleu commented Aug 11, 2023

rodrigogiraoserrao commented Aug 17, 2023

		for widget in WIDGETS.keys():
		yield Button(widget, id=widget)

		for widget_name, widget in WIDGETS.items():
		yield from widget(id=widget_name)

Draft: Implement command widgets to textual CLI #8

Are you sure you want to change the base?

Draft: Implement command widgets to textual CLI #8

Conversation

pavelkraleu commented Jul 23, 2023 • edited Loading

rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelkraleu commented Aug 11, 2023

rodrigogiraoserrao commented Aug 17, 2023

pavelkraleu commented Jul 23, 2023 •

edited

Loading