Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: fix TypeError when running test.pebble_cli #1245

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

benhoyt
Copy link
Collaborator

@benhoyt benhoyt commented Jun 5, 2024

Without this fix, running this raises the following:

TypeError: catching classes that do not inherit from BaseException is not allowed

Not sure how to silence pyright without the type: ignores.

I guess we haven't used this script in a while. :-)

Without this fix, running this raises the following:

TypeError: catching classes that do not inherit from BaseException is not allowed
@benhoyt benhoyt requested review from tonyandrewmeyer and dimaqq June 5, 2024 02:59
@benhoyt benhoyt changed the title fix: fix TypeError when running test/pebble_cli.py fix: fix TypeError when running test.pebble_cli Jun 5, 2024
Copy link
Contributor

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

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

Given that it’s a script and typing issue get fixed…

I don’t quite understand the original type hint though.

@tonyandrewmeyer
Copy link
Contributor

tonyandrewmeyer commented Jun 5, 2024

How do you reproduce the error?

I've started pebble with:

PEBBLE=/tmp/pebble go run ./cmd/pebble run

and running test/pebble_cli from main seems ok?

$ PEBBLE=/tmp/pebble /tmp/opsenv/bin/python3 test/pebble_cli.py system-info
SystemInfo(version='v1.10.0-dev')
$ /tmp/opsenv/bin/python3 --version
Python 3.12.3

Are you using a different Python? Or should I run it differently?

@benhoyt
Copy link
Collaborator Author

benhoyt commented Jun 5, 2024

@tonyandrewmeyer You have to run an exec command specifically. I can repro it by starting Pebble, then running this:

$ .tox/unit/bin/python -m test.pebble_cli --socket=/var/lib/pebble/default/.pebble.socket exec -- echo foo
foo
Traceback (most recent call last):
  File "/home/ben/w/operator/test/pebble_cli.py", line 248, in main
    sys.exit(0)
SystemExit: 0

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/home/ben/w/operator/test/pebble_cli.py", line 316, in <module>
    main()
  File "/home/ben/w/operator/test/pebble_cli.py", line 250, in main
    except pebble.ExecError[str] as e:
TypeError: catching classes that do not inherit from BaseException is not allowed

Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

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

I have the feeling I maybe introduced this - when I was adding hints to all the test files the older version of pyright seemed to want the exceptions like this, although it's clearly invalid.

I also don't know of any way to say "catch this exception and I know that it will be this concrete type". So I think the first ignore is required.

The second ignore could be removed if you have a e = typing.cast(pebble.ExecError[str], e) but I think having the ignore there is fine in this context.

It feels like, just as with Python typing in many places, having generic exception classes doesn't work very well.

@benhoyt benhoyt merged commit dcbe21b into canonical:main Jun 5, 2024
26 checks passed
@benhoyt benhoyt deleted the fix-pebble-cli branch June 5, 2024 04:40
tonyandrewmeyer pushed a commit to tonyandrewmeyer/operator that referenced this pull request Jun 26, 2024
Without this fix, running this raises the following:

```
TypeError: catching classes that do not inherit from BaseException is not allowed
```

Not sure how to silence pyright without the type: ignores.

I guess we haven't used this script in a while. :-)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants