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

refactor: remove conditional type checking #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

blueraft
Copy link
Collaborator

Resolves #15

@hampusnasstrom
Copy link
Contributor

Are you guys sure we should do this? We've had a lot of problems in the past with both the structlog dependency and circular imports with EntryArchive. I would like to really test this thoroughly in different developing environments and with all our plugins before merging this. Also, shouldn't this then be addressed in the nomad-lab source code first? https://gitlab.mpcdf.mpg.de/nomad-lab/nomad-FAIR/-/blob/develop/nomad/datamodel/metainfo/basesections.py?ref_type=heads#L238

@hampusnasstrom
Copy link
Contributor

@markus1978 do you remember why we settled on using the string imports and the type checking if statement? I'll try to dig up the issue but might take some time...

@hampusnasstrom
Copy link
Contributor

This was the MR: gitlab.mpcdf.mpg.de/nomad-lab/nomad-FAIR/-/merge_requests/1350. Might be that we had the discussion on Slack.

@blueraft
Copy link
Collaborator Author

why we settled on using the string imports and the type checking if statement?

Based on that MR, probably because structlog was installed as an extra in parsing but the MR removes installing with parsing, so the import will cause a run time error without conditional type checking.

Are you guys sure we should do this?

I don't feel strongly about this. What we have in main is the safer choice, but the change in the PR will simplify the code a bit. @lauri-codes what do you think?

@lauri-codes
Copy link
Collaborator

With the tests I did, I could not see any issues with imports, but probably my tests did not cover everything. Do you @hampusnasstrom think you can create a breaking example...?

Let's keep this MR open until we have some certainty. But essentially the TYPE_CHECKING is an escape hatch for cyclical dependencies, an issue that we hope to be gone with the new plugin system.

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.

Definitions for type-checking
3 participants