Nadużywanie wartości domyślnych parametrów funkcji

[dybi]

Oprócz tego, myślę, że warto byłoby napisać o NIE nadawaniu wartości domyślnych wszystkim argumentom w funkcjach. W kodzie mamy sporo funkcji typu:

def weird_fun(param1=None, param2=None):
    assert param1 is not None
    assert isinstance(param2, str)
   # some code 

[cameel]

Taka drobna meta-uwaga: utworzyłem labele dla poszczególnych narzędzi (python, shell, git, markdown) i części naszego guide'a (coding style, workflow), więc nie trzeba tego pisać w nazwie issue. Korzystajmy z tych labeli.

[igsj]

Jeśli ktoś definiuje def fun(param=None).. mówiąc przez to brak param jest ok i może być None i zaraz potem dodaje assert param is not None to w Coding Style przyda się sugestia, żeby wyjść na krótki spacer od czasu do czasu. Bo chyba dodanie reguły Nie pisz kodu, który sam sobie zaprzecza w dwóch kolejnych linijkach wydaje się mało empatyczne.

[cameel]

Hehe :), no ten przykład jest na tyle ekstremalny że można by go uznać za buga.

Ale ogólnie moim zdaniem jest wiele mniej oczywistych sytuacji i istnieje tendencja by na siłę dawać domyślną wartość dla wygody. Według mnie reguła powinna mówić, że unikamy nadawania wartości domyślnej tylko dlatego, że jakaś wartość parametru jest częściej używana niż inna i nie chce nam się jej ciągle pisać. Nadajemy je raczej w sytuacjach gdy dany parametr jest po prostu opcjonalny. Stąd też wynika to zalecenie, które już tam jest - żeby unikać wartości domyślnych innych niż pusty string czy None. Bo kiedy parametr jest opcjonalny to zwykle pomijamy go ustawiając None.

[rwrzesien]

:+1: z tym że

W kodzie mamy sporo funkcji typu:

Nie znalazłem ani jednej.

[dybi]

@rwrzesien , nie dokładnie takich funkcji, ale tego typu ;)

Żeby nie szukać daleko:

def update_subtask(
    subtask:                        Subtask,
    state:                          Subtask.SubtaskState,
    next_deadline:                  int                                  = None,
    set_next_deadline:              bool                                 = False,
    task_to_compute:                message.TaskToCompute                = None,
    report_computed_task:           message.ReportComputedTask           = None,
    ack_report_computed_task:       message.AckReportComputedTask        = None,
    reject_report_computed_task:    message.RejectReportComputedTask     = None,
    subtask_results_accepted:       message.tasks.SubtaskResultsAccepted = None,
    subtask_results_rejected:       message.tasks.SubtaskResultsRejected = None,
):
    """
    Validates and updates subtask and its data.
    Stores related messages in StoredMessage table and adds relation to newly created subtask.
    """
    assert isinstance(subtask, Subtask)
    assert state in Subtask.SubtaskState
    assert (state in Subtask.ACTIVE_STATES)  == (next_deadline is not None)
    assert (state in Subtask.PASSIVE_STATES) == (next_deadline is None)

next_deadline jest deklarowany jako int i przypisywana jest mu wartość domyślna None.

Podobnie:

def store_or_update_subtask(
    task_id:                        str,
    subtask_id:                     str,
    provider_public_key:            bytes,
    requestor_public_key:           bytes,
    state:                          Subtask.SubtaskState,
    next_deadline:                  int                                  = None,
    set_next_deadline:              bool                                 = False,
    task_to_compute:                message.TaskToCompute                = None,
    report_computed_task:           message.ReportComputedTask           = None,
    ack_report_computed_task:       message.AckReportComputedTask        = None,
    reject_report_computed_task:    message.RejectReportComputedTask     = None,
    subtask_results_accepted:       message.tasks.SubtaskResultsAccepted = None,
    subtask_results_rejected:       message.tasks.SubtaskResultsRejected = None,
):

Messsage są zadeklarowane jako obiekty odpowiednich klas, a przypisywana jest im wartość domyślna None.

Wniosek: Nie korzystamy w kodzie z adnotacji Optional[some_type] i warto byłoby to zmienić ;)

EDIT: A dokłądnie takie widziałem w niektórych naszych concentowych PR-ach ;)

[dybi]

Kolejnym przykładem na nadużywanie wartości domyślnych jest funkcja:

def store_pending_message(
    response_type       = None,
    client_public_key   = None,
    queue               = None,
    subtask             = None,
    payment_message     = None,
):

Wszystkie jej argumenty są domyślnie Nonem, a we wszystkich jej wywołaniach przekazywane są parametry: response_type, queue i client_public_key

[dybi]

Ok, propozycja do wiki:

• Funkcje powinny zawierać wartości domyślne dla argumentów wtedy i tylko wtedy gdy ma to sens, tzn. gdy faktycznie możemy wywołać funkcję bez danego argumentu, bo wartość domyślna "załatwi sprawę".
• Deklarowany typ argumentu musi być zgodny z wartością domyślną.
• W przypadku wartości domyślnych None (jakaś wartość może wystąpić lub nie) używamy deklaracji Optional[Typ].

[rwrzesien]

@dybi :+1:

store_pending_message - tutaj z tego co pamiętam na początku nie wszędzie były, później zaczeliśmy uzupełniać.

update_subtask i store_or_update_subtask - tutaj akurat te asserty mają sens, ponieważ next_deadline może być None tylko dla pewnych stanów Subtaska.