Skip to content

Conversation

@gmargaritis
Copy link
Contributor

No description provided.

@gmargaritis gmargaritis requested a review from a team January 26, 2024 16:47
Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @gmargaritis! I have just a couple of comments.

@gmargaritis
Copy link
Contributor Author

gmargaritis commented Jan 30, 2024

@parisk We are now getting the following error from mypy:

ergani/models.py:229: error: Incompatible types in assignment (expression has type "list[EmployeeWeeklySchedule]", base class "CompanyDailySchedule" defined the type as "list[EmployeeDailySchedule]")  [assignment]
ergani/models.py:229: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
ergani/models.py:229: note: Consider using "Sequence" instead, which is covariant

Also, in VSCode if I hover over CompanyWeeklySchedule I get the following:

class CompanyWeeklySchedule(
    business_branch_number: int,
    start_date: date = None,
    end_date: date = None,
    employee_schedules: List[EmployeeWeeklySchedule] = list,
    related_protocol_id: str | None = "",
    related_protocol_date: date | None = None,
    comments: str | None = ""
)

In dataclasses, we cannot override the default values1 of start_date and end_date, even though they are not optional. Does this affect us in any way? Do you think that we should ditch inheritance altogether?

Footnotes

  1. https://stackoverflow.com/a/53085935

@parisk
Copy link
Contributor

parisk commented Jan 30, 2024

@gmargaritis I thought that maybe we could give a go at multiple inheritance and mixing, but it is getting way too complex to make sense. I suggest we just hard-code all attributes and opt-in to repeat ourselves in this case. What are your thoughts?

@gmargaritis gmargaritis force-pushed the introduce-multiple-submissions branch from 76784ae to 4b6f6a5 Compare January 30, 2024 14:34
@gmargaritis gmargaritis requested a review from parisk January 30, 2024 14:34
Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🦒

@gmargaritis gmargaritis merged commit eb0d610 into main Jan 30, 2024
@gmargaritis gmargaritis deleted the introduce-multiple-submissions branch January 30, 2024 14:55
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.

3 participants