Skip to content

Fix IsMoreThanNthGenerationDescendantOf.init_list() typing #2010

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

Open
wants to merge 1 commit into
base: maintenance/gramps60
Choose a base branch
from

Conversation

stevenyoungs
Copy link
Contributor

@stevenyoungs stevenyoungs commented Mar 8, 2025

The person argument of IsMoreThanNthGenerationDescendantOf.init_list() is typed to expect a Person
From IsMoreThanNthGenerationDescendantOf.prepare() a parameter of type DataDict | None was passed.
From within IsMoreThanNthGenerationDescendantOf.init_list a recursive call is made passing a Person object.

Unify so that the parameter always has the type DataDict | None

@Nick-Hall
Copy link
Member

Most of the filter init_list methods seem to take a primary object rather than a DataDict. Have look at the init_list method in the IsLessThanNthGenerationDescendantOf rule.

I think that we should leave this until v6.1 unless there is a real bug associated with it.

@stevenyoungs
Copy link
Contributor Author

Most of the filter init_list methods seem to take a primary object rather than a DataDict. Have look at the init_list method in the IsLessThanNthGenerationDescendantOf rule.

I chose to standardise on DataDict as it will be lightly faster.
I think init_list is an internal method so we do not need to be consistent across the different classes. In this class a DataDict is sufficient. In other filters, the true object may be required.
There's a general decision to be made here. A DataDict and the true object are designed to be interchangeable. As typing is introduced we need guidance on what pattern to follow in situations where a DataDict provides sufficient functionality:

  • person: Person
  • person: DataDict
  • person: Person | DataDict

The last option offers the most flexibility, allowing the calling code to provide whichever type is most convenient, at the expense of a little more typing when defining the method.

I think that we should leave this until v6.1 unless there is a real bug associated with it.

Agreed. So long as we do not make any other code change that exposes the lint error in 6.0, no need to fix in 6.0.

@dsblank
Copy link
Member

dsblank commented Mar 12, 2025

If we make Person be a protocol (like we're thinking Database should be) then that might work. Unless one is importing both gen.lib.Person and a new typing.Person. I guess they might need different names.

@Nick-Hall Nick-Hall added this to the v6.1 milestone Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants