Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving get handlers and agnosticID behaviour #84

Open
Toma400 opened this issue Oct 6, 2023 · 0 comments
Open

Improving get handlers and agnosticID behaviour #84

Toma400 opened this issue Oct 6, 2023 · 0 comments
Labels
code improvement Call for some improvement of code feature request Feature being officially approved to be added high priority Label for requests being highly prioritised

Comments

@Toma400
Copy link
Owner

Toma400 commented Oct 6, 2023

While doing work on 0c97029, I realised that some systems are not properly coded, despite them running fine after some tweaking on my side. And so, even if this makes no big deal right now, when issues is closed with commit above, there should be further look into two sections & all functions using those:

  1. Getters
    Getters in various classes, profession.py Class, race.py Race and so on tend to have no way to check whether key is or is not in their system. While this passes the responsibility explicitly to caller function (such as here), this also obstructs important cases where it would be important to know whether this specific key is missing.
    This may be me just not sure on current design, because obviously we don't want caller to decide on KeyErrors and return None (it is much more annoying to handle exceptions, as we do it twice for issues made on None type as addition). Putting it as caller reponsibility seems purposeful, as if we don't expect KeyErrors, we just don't do try/except, or we can put log entry into it and reraise.
    It's probably issue part which should be just discussed whether there are better alternatives or if it's done the best way we could.
    Also checking uses of each getter would be nice, so we can put try/except clauses wherever it is needed.

  2. AgnosticID
    While getAttributesTupleAdjusted in attributes.py uses system that converts vague IDs into absolute ones, getAttribute (used so far only by function above, without issues) uses agnosticID converter, which seems to me to be totally out of place - the sole existence of agnosticID function was to pass vague IDs into absolute ones, yet somehow I passed this function to do exactly reverse work.
    So the question here is to advocate agnosticID against absoluteID converters and either find use for the former, or to unify them into absoluteID functionality. As I have really no idea what I wanted from agnosticID when I created it, or if I just were confused.

@Toma400 Toma400 added feature request Feature being officially approved to be added code improvement Call for some improvement of code high priority Label for requests being highly prioritised labels Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code improvement Call for some improvement of code feature request Feature being officially approved to be added high priority Label for requests being highly prioritised
Projects
None yet
Development

No branches or pull requests

1 participant