search

FAIRmat-NFDI / cookiecutter-nomad-plugin

MIT License
1 stars 1 forks source link

refactor: remove conditional type checking #16

Open blueraft opened 4 months ago

blueraft commented 4 months ago

Resolves #15

hampusnasstrom commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.