Skip to content

feat: transition 'local' command to tradeorm for speed#266

Draft
kfsone wants to merge 1 commit intorelease/v1from
dev/local-cmd-orm-refactor
Draft

feat: transition 'local' command to tradeorm for speed#266
kfsone wants to merge 1 commit intorelease/v1from
dev/local-cmd-orm-refactor

Conversation

@kfsone
Copy link
Collaborator

@kfsone kfsone commented Jan 2, 2026

DRAFT:

Transitions "local" command to a new pattern that makes it much more responsive, adding a few extra options and some cleanup.

new "--limit" option:
image

systems with stations that are not fleet carriers within 8 ly:
image

within 6 ly of reorte, systems + stations that are trading and known to be within 300 ls of arrival point, non-planetary, and non-fleet carrier:
image

stations in "Rhea" (ly=0) which aren't planetary:
image

@kfsone kfsone force-pushed the dev/local-cmd-orm-refactor branch 2 times, most recently from b381a9c to 28b5ab1 Compare January 2, 2026 01:48
@kfsone
Copy link
Collaborator Author

kfsone commented Jan 2, 2026

Made a bit more use of --detail here:

image

@kfsone kfsone force-pushed the dev/local-cmd-orm-refactor branch from 28b5ab1 to 790e151 Compare January 2, 2026 08:47
@Tromador
Copy link
Collaborator

Tromador commented Jan 3, 2026

local’s “unknown system” handling doesn’t match TradeORM.lookup_system behaviour

TradeORM.lookup_system raises on unknown systems; it does not return None, so the new local_cmd.run()’s if not origin: branch is effectively dead, and the exception type is not the CommandLineError it tries to raise.
In TradeORM.lookup_system

result: orm.Station | orm.System | None = self.lookup_place(system_name)
...
if sys_name:
    system = self._system_lookup(sys_name)
    if not system:
        raise TradeException(f"unknown system: {sys_name}")

then in local_cmd.py:run

origin = tdb.lookup_system(cmdenv.near)
if not origin:
    raise CommandLineError(f"Unknown system: {cmdenv.near}")

Impact: you’ll get a TradeException bubbling up (whatever the global handler does with it), not the intended CommandLineError with consistent CLI messaging.

Same duplicate uprint here as in #263

Might be more, but I'm checking in case I'm seeing ghosts.

@kfsone
Copy link
Collaborator Author

kfsone commented Jan 3, 2026

note: this branch is a continuation of the max-link-ly one, I was just too lazy to split them totally

@kfsone kfsone force-pushed the dev/local-cmd-orm-refactor branch 2 times, most recently from 9610810 to 8d2bb42 Compare January 3, 2026 03:40
@kfsone
Copy link
Collaborator Author

kfsone commented Jan 3, 2026

duplicate uprint fixed in parent branch.

That leaves the exception handling, which I think is a testament to TradeORM lookup methods needing fleshing out. TradeException is probably the wrong thing (too broad?) for them to throw, but equally KeyError would mean cluttering every call site with a try/catch wrapper.

Python convention seems to be to allow you to specify a default to indicate no-throw:

lookup_system("kfsone")   <- raise
lookup_system("kfsone", None)  <- return None
lookup_system("kfsone", default=None) <- return None

Alternative 1: Add a specific "NameNotFoundError", 'Could not match {typename}: '{name}'', so catching is an option rather than a mandate (if you want to give the user good feedback).

Alternative 2: (can include alt 1) try-unless-default as above?

@kfsone
Copy link
Collaborator Author

kfsone commented Jan 8, 2026

I think @Tromador's points about lookup are a foundational aspect of the DB->ORM work and thus #245.

Does it make most sense to draft this PR and gate it's return on work to reach acceptable feature-parity between TradeORM.lookup_place and TradeDB.lookup_place?

@Tromador
Copy link
Collaborator

I think @Tromador's points about lookup are a foundational aspect of the DB->ORM work and thus #245.

Does it make most sense to draft this PR and gate it's return on work to reach acceptable feature-parity between TradeORM.lookup_place and TradeDB.lookup_place?

Yeah, I think that makes sense. Need the right foundation to build on.

@kfsone kfsone marked this pull request as draft January 10, 2026 08:41
@kfsone kfsone changed the title Dev/local cmd orm refactor feat: transition 'local' command to tradeorm for speed Jan 24, 2026
@kfsone kfsone force-pushed the dev/local-cmd-orm-refactor branch 2 times, most recently from 35c958f to 72c32ef Compare January 24, 2026 01:53
feat: makes 'local' much faster,
chore: minor cosmetic improvements,

feat: ORM learnings from porting another command
- TradeORM has it's own Pythonic (all-caps) constants for mapping Y/N/? to labels" TRISTATE_LABELS, MARKET_STATES, PLANET_STATES, FLEET_STATES, ODYSSEY_STATES, and PADSIZE_LABELS.
- added some extra orm-query helpers to Station, and made some 'dbname' properties hybrids so they can be used in queries
- - Station.is_fleet_carrier: bool, fleet_carrier: str, is_planetary: bool (includes horizons and odyssey planets), is_odyssey_planetary: bool), odyssey: str,
- type hinting/alignment in trade_cmd
- make more use of --detail, move age to -vv
@kfsone kfsone force-pushed the dev/local-cmd-orm-refactor branch from 72c32ef to 67ce85f Compare January 24, 2026 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants