Skip to main content
added 1 character in body
Source Link
J_H
  • 42.1k
  • 3
  • 38
  • 157

Please__don't__use__double_dunder__scores__in__variable__namesPlease__don't__use__double__dunder__scores__in__variable__names. It's just weird, and doesn't have any meaning. It's not like you wrote __username__ or __str__, which describe protocols that are special for the cPython interpreter. Pick normal names, please.

Please__don't__use__double_dunder__scores__in__variable__names. It's just weird, and doesn't have any meaning. It's not like you wrote __username__ or __str__, which describe protocols that are special for the cPython interpreter. Pick normal names, please.

Please__don't__use__double__dunder__scores__in__variable__names. It's just weird, and doesn't have any meaning. It's not like you wrote __username__ or __str__, which describe protocols that are special for the cPython interpreter. Pick normal names, please.

Source Link
J_H
  • 42.1k
  • 3
  • 38
  • 157

"todo with undo"

Ok, I just kind of love that!

modern type annotations

def list_tasks(  ...
) -> typ.Union[ResponsePayload, typ.Dict[str, typ.List[ErrorDetail]]]:

Prefer the | syntax for these, e.g.
-> ResponsePayload | dict[str, list[ErrorDetail]]:

The notation you're using is certainly valid, but it is rather dated, consistent with cPython interpreters of prior years, maybe IDK 3.7 or so.

parameters are part of the Public API

def list_tasks(  ...
    ...
    _page_number: int = 1,  # Page number
    _per_page: int = 10,  # Number of items per page
    ...

I don't understand what meaning the _ underscore _local prefix could possibly indicate in this context. Callers will dance to the tune you call, they will conform to the Public API you are designing. To put foo in the public signature and then change your mind and label it _foo doesn't make sense. Make these local variables if you believe caller should never specify their value.

BTW, thank you for the very nice type annotation on errors. Prefer to spell it errors: list[ErrorDetail] = [] since the rest of your code indicates that we're targeting sufficiently modern interpreters which understand such annotations.

And rather than e.g. Optional[date], prefer date | None.

dealing with errors

    except ValueError as e:
    ...
    except ValueError as e:
    ...
    except ValueError as e:
    ...
    except ValueError as e:

IDK, maybe we need all these?

Maybe better to attempt four actions within a single try?

The big thing I'm not yet clear on is, what is the Single Responsibility of this method, what promise does it make to the caller, what post-condition does it guarantee?

We attempt a first, second, and third action which could potentially fail. In the event of failure, it's not obvious to me that we are required to soldier on and attempt the third or fourth action. Would bailing out early be acceptable? The """docstring""" doesn't offer any advice on this question, since it is missing. Please write one. In the coming months, maintenance engineers will want to know what the various responsibilities are. They could wing it, making random choices, but you might not be happy with what they choose. Offer some guidance.

local variable

    # Stick with class not using dictionary.
    _list_tasks = []

I don't understand what that means.

You're defining a local variable. It is fundamentally "private"; we don't need a _private prefix on it.

It's not like we're assigning self._list_tasks, which caller might possibly mess with. This local variable is guaranteed to go out-of-scope and be destroyed by the time caller gets to interact with its results. That's why we create small functions in the first place: so local vars are created and destroyed by the time we've computed some useful result and returned it.

double dunder

What is going on here?

        user_instance = (
            validate_username(created_by__username) if ...
        )

That created_by__username parameter was part of the signature -- I didn't even notice that craziness.

Please__don't__use__double_dunder__scores__in__variable__names. It's just weird, and doesn't have any meaning. It's not like you wrote __username__ or __str__, which describe protocols that are special for the cPython interpreter. Pick normal names, please.