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

Can document.objects contain non-TopLevel objects? #295

Open
jakebeal opened this issue Aug 28, 2021 · 3 comments
Open

Can document.objects contain non-TopLevel objects? #295

jakebeal opened this issue Aug 28, 2021 · 3 comments
Milestone

Comments

@jakebeal
Copy link
Contributor

I just noticed that document.objects is of type List[Identified], rather than List[TopLevel] like I had expected it would be. Is there a case in which it can contain a non-TopLevel object? If so, what is it? If not, can we change it to List[TopLevel]?

@jakebeal jakebeal added the question Further information is requested label Aug 28, 2021
@bbartley
Copy link
Contributor

If I remember correctly, the object store temporarily contains Identified objects during loading of a document. The objects are first deserialized into this store as a flat list, and then subsequently structured into a tree.

Since the type hints are not enforced though, we could still change the type hint to indicate List[TopLevel]

@jakebeal
Copy link
Contributor Author

I think they may go into document.orphans instead of document.objects in any case?

@tcmitchell
Copy link
Collaborator

Document.objects should ultimately contain only instances of TopLevel, so List[TopLevel] is likely a better type annotation. It may be the case, as @bbartley says, that some non-TopLevel objects are temporarily in Document.objects as the loading goes through its phases. These objects will end up in Document.orphans eventually, but early in the loading process we don't know they are orphans.

We should make this change to the type annotation and we should ensure that Document.objects always and only contains instances of TopLevel. This may require the use of an additional temporary list somewhere to avoid writing to Document.objects.

@tcmitchell tcmitchell added this to the beta milestone Aug 31, 2021
@jakebeal jakebeal modified the milestones: beta, 1.1 Feb 8, 2022
@jakebeal jakebeal removed the question Further information is requested label Feb 10, 2022
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

No branches or pull requests

3 participants