Skip to content

Two location guards test presence rather than lock, so alt/az can be computed at 0,0 #656

Description

@brickbots

Found while verifying the wording for #646 (docs PR #653). Latent — no user-visible symptom today — but the guards do not express what they are trying to express, and the thing masking them is upstream and unrelated.

The problem

Two call sites guard on the presence of a Location when what they need is a locked one.

python/PiFinder/calc_utils.py:243calc_object_altitude():

location = shared_state.location()
dt = shared_state.datetime()
if location and dt and solution:
    aa = FastAltAz(location.lat, location.lon, dt)

python/PiFinder/integrator.py:493_get_alt_az():

if ra_deg is None or dec_deg is None or location is None or dt is None:
    return None, None
calc_utils.sf_utils.set_location(location.lat, location.lon, location.altitude)

Why neither guard does anything

Location is a plain @dataclass (state.py:196) with no __bool__ and no __len__, so every instance is truthy. And the field is initialized to a real object, not None:

self.__location: Location = Location()     # state.py:295

whose defaults are lat = 0.0, lon = 0.0, lock = False. So if location is always true, and location is None is essentially always false.

The guards therefore reduce to if dt and solution and if ra/dec and dt. A path that sets a datetime without a lock computes alt/az for an observer at 0°N 0°E — Null Island, in the Gulf of Guinea. The result is a plausible-looking number that is wrong by however far the user is from there.

Why nothing breaks today

Both are reached through UI paths already gated on shared_state.altaz_ready() (state.py:332), which does test location.lock. The masking is upstream and incidental — nothing in either function records that it depends on a caller checking lock first.

Suggested fix

Guard on location.lock at both sites, matching altaz_ready(). Defining __bool__ on Location to mean "locked" would make the existing if location read correctly, but that is a wider change with its own trap: if location would then be false for a perfectly valid unlocked Location, and state.py:544 (if self.__location and self.__location.timezone) relies on current truthiness for timezone resolution, which has nothing to do with lock. Explicit location.lock at the two call sites is the smaller and clearer change.

Note set_location() (state.py:471) does assign v unconditionally, so None is technically reachable through it. Keeping a None check alongside a lock check is defensible; testing only for None is not.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions