search

canonical / craft-application

The basis for *craft applications
https://canonical-craft-application.readthedocs-hosted.com/en/latest
GNU Lesser General Public License v3.0
7 stars 10 forks source link

Use predefined exit codes #379

Open mattculler opened 1 month ago

mattculler commented 1 month ago

What needs to get done

In application.py (as well as errors.py and services/remotebuild.py) there are hardcoded constants that refer to sysexits.h. These constants are already defined in the python standard library under os: https://docs.python.org/3/library/os.html#os.EX_OK

More pedantic, less important: PartsError and ProviderError both exit 1 - whether these are changed to something more distinctive or not, these should be defined publically, so the values can be used in unit tests. CraftError exits an overrideable value that it defines internally as defaulting to 1, this can also be exposed for tests.

Why it needs to get done

Using predefined values is better practice than using magic numbers with explanatory comments.

thp-canonical commented 1 month ago

There's another two instances that return 128 + signal.SIGINT:

https://github.com/canonical/craft-application/blob/2a924df5d31869079b8c8f901cfc15159f27fc06/craft_application/application.py#L417

https://github.com/canonical/craft-application/blob/2a924df5d31869079b8c8f901cfc15159f27fc06/craft_application/application.py#L544

Maybe a definition like this somewhere can be used to refer to 128 + signal.SIGINT with a proper name?

EX_KEYBOARD_INTERRUPT = 128 + signal.SIGINT

Not sure if there's a better "name" for 128 + signal.SIGINT, A quick google search reveals this and in turn this.

lengau commented 1 month ago

Oh nice - I didn't know about those constants. We can replace some of these cases, but not the specific one mentioned without adding our own set or using a library.

I've created #387 to handle the simple cases, but I'll have to think about the 128+blah ones. The place where @thp-canonical points out is indeed where I got that value.