Follow-up from "Wkedzierski/196 recurrent transfer"

The following discussions from !374 (merged) should be addressed:

@mzebrak started a discussion: (+2 comments)

Why are they common options dataclasses when they are not shared between multiple commands and I think they'll never be because they are very context-specific (transfer schedule)?
@mzebrak started a discussion:

Instead of overriding run, wouldn't it be better to use a dedicated interface and provide a logic for validate and configure? That's what you are overriding run for.
@mzebrak started a discussion: (+4 comments)

Why is there so much code duplication? Looks like mainly caused by the usage of DecimalConverter for integers. We don't need to convert str (which should be integer) via DecimalConverter I think?

Message should be same as when some option is typed as : int (default message from typer itself)
@mzebrak started a discussion:

Please see: !329 (comment 162575)

(mainly I see points 1. (for Remove cmd) and 2. here)
@mzebrak started a discussion:

Why like that? I think schemas have a model for that (unfortunately i think not with the type, value form but you can use it for id validation and extension_name for not hardcoding this name)

Also I'm not sure about the if pair_id > 0 - will be changed after hardfork to be included always? Maybe add a # TODO: here?
@mzebrak started a discussion:

this comaprison of self.from == transfer.from AND self.to == transfer.to AND self.pair_id == transfer.pair_id should be placed in some method like identify_transfer or something
@mzebrak started a discussion:
boolean params should be kwargs only.
```
    def validate_existence(self, scheduled_transfer: TransferSchedule | None, *, should_exists: bool) -> None:
```
see !356 (comment 163795)

Also when there is self.scheduled_transfer available why pass it via param and not use self for that?
@mzebrak started a discussion:

Maybe just RecurrentTransfer instead of TransferSchedule? it's not quite a schedule because it contains also info about transfer definition (I'd say everything)
@mzebrak started a discussion:
1. So this dataclass is like Enum? or Literal? Maybe literal will suit the best? We tend to use it instead enum
2. It is placed there but actual sort_by is placed in clive/__private/cli/commands/show/show_transfer_schedule.py ?? I think sort logic could be stored in this FindScheduledTransfers instead of in ShowTransferSchedule, similar like https://gitlab.syncad.com/hive/clive/blob/1c287b8a551ed90873bb59c91629c763dc31fc4c/clive/__private/core/commands/data_retrieval/proposals_data.py#L64
this has the benefit sorting will be available among all interfaces (TUI/CLI/python) and not only in CLI
@mzebrak started a discussion:

Maybe can you not hardcode HIVE/HBD token and use Asset.get_token and Asset.LiquidT?
@mzebrak started a discussion:

why not compare timedelta with another timedelta? much cleaner
@mzebrak started a discussion:

Instead of having lots of type ignores there I think mypy will get it right if you remove typealias and just use Callable[P, R] instead of RenamedFuncT.
@mzebrak started a discussion: (+8 comments)

this should be adjusted

looks like you dont need this wrapper

        scheduled_transfers = (await self.world.commands.find_scheduled_transfers(account_name=self.account_name)).result_or_raise

@mzebrak started a discussion:

                str(scheduled_transfer.pair_id),

@mzebrak started a discussion:
something like
```
Sortable = TypeVar("Sortable", ScheduledTransfers, "FutureScheduledTransfers")
```
and
```
    def get_sorted_by(self, scheduled_transfers: Sortable, sort_by: list[str]) -> Sortable:
```
will do the same job because mypy will see this input and output tied together so when you pass FutureScheduledTransfers it knows output will be also FutureScheduledTransfers
@mzebrak started a discussion:

extract this to a variable
@mzebrak started a discussion:

why is there this type ignore? could you get rid of it? same for a few lines below
@mzebrak started a discussion:

same in ProcessTransferScheduleModify
@mzebrak started a discussion:

WEould be good to have that and possible_amount_aligned in some more general place like https://gitlab.syncad.com/hive/clive/blob/5b72d4107a2ce475112862e7158389ddb0d1b00d/clive/__private/core/commands/data_retrieval/savings_data.py#L66 does.

Maybe this can be resolved with other thread mentioning sorting should be placed in Command and not CLICommand (it could return own dataclass on which we can add such a method
@mzebrak started a discussion

hardcoded, but constant could be used

Edited Jul 02, 2024 by Mateusz Żebrak