Add type hints for base.py

[hippalectryon-0]

Follows #416

Comments / Open questions

• In qubesd_call we had

 if not self.app:
            raise NotImplementedError

I was unable to find any case in which self.app could be None in the first place, so I removed it.

• right after we have

dest: QubesVM | None = getattr(self.app, "default_dispvm", None)
return self.app.qubesd_call(dest, method, arg, payload, payload_stream)

but qubesd_call expects a non-None dest. I had to add an assert dest is not None, but what prevents it from being None in the first place here ?

• Error type:

        if prop_type == 'bool':
            if value == '':
                return AttributeError

Shouldn't we be returning a ValueError ?

[codecov]

Codecov Report

❌ Patch coverage is 98.21429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.27%. Comparing base (c6b516d) to head (7cc0bec).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
qubesadmin/base.py	98.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
+ Coverage   76.24%   76.27%   +0.02%     
==========================================
  Files          53       53              
  Lines        9325     9336      +11     
==========================================
+ Hits         7110     7121      +11     
  Misses       2215     2215              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marmarek]

Shouldn't we be returning a ValueError ?

Makes sense. Generally, this means a bug somewhere in qubesd.

[marmarek]

but qubesd_call expects a non-None dest.

Most calls are going through PropertyHolder.qubesd_call, and it has handling for dest = None (well, almost, see discussion in the other PR).

As for QubesLocal/QubesRemote, it's an error to have None there, adding assert is okay. @ben-grande already found one case that, if such assert would be present, would be much easier to debug: #438 (comment)

[hippalectryon-0]

Makes sense. Generally, this means a bug somewhere in qubesd.

After further investigation this is intentional - since this is a wrapper to access named field <prop_type>, it should indeed raise an AttributeError if the prop_type doesn't exist

[marmarek]

You mean a case where property is declared but not set? Like for example stubdom_mem for PVH? Yeah, indeed in that case qubesd may return an empty value... The type situation here is not great, you get type=int, but empty string as serialized value. But then, if you have type=str, there is no way to express that situation...

[hippalectryon-0]

Regarding the discussion about dest being None, after re-reviewing this PR, I don't think there's anything blocking left - the required work (if any) will be done in other PRs.

Can you confirm if there's any other open comments/... for this PR ?

[marmarek]

In that case please remove the TODO comment about AttributeError

[hippalectryon-0]

done

[marmarek]

should be okay now, but I'd like to wait for CI to become green first.