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

Fix breakage caused by changes in formulaic #423

Merged
merged 3 commits into from
Dec 4, 2024
Merged

Fix breakage caused by changes in formulaic #423

merged 3 commits into from
Dec 4, 2024

Conversation

stanmart
Copy link
Collaborator

@stanmart stanmart commented Dec 4, 2024

It seems like formulaic might be considering the EncodedTermStructure a private class based on the fact that it is being moved in a non-major release. A bigger refactor of the formula module might be worth considering at some point now that formulaic reached 1.0 and is more stable. Until then, this PR provides a quick fix for #422.

Checklist

  • Added a CHANGELOG.rst entry

Comment on lines +26 to +29
try:
from formulaic.materializers.base import EncodedTermStructure
except ImportError:
from formulaic.materializers.types.formula_materializer import EncodedTermStructure
Copy link
Member

Choose a reason for hiding this comment

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

Could we use the formulaic version to select the import path instead of a try/except block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately I don't know in which version the change will appear. It's on main now, and my guess is 1.1, but I'm not sure.

Copy link
Member

@lbittarello lbittarello left a comment

Choose a reason for hiding this comment

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

Maybe raise an issue with formulaic about breaking the public API without a major release?

@MarcAntoineSchmidtQC
Copy link
Member

@stanmart, I hijacked your PR to also update a CI runner so that the CI would pass.

@MarcAntoineSchmidtQC
Copy link
Member

oops, I thought it would be a simple fix. Not the case.

I will open a different PR to fix this.

@stanmart
Copy link
Collaborator Author

stanmart commented Dec 4, 2024

haha, all good, I'll merge it then

@stanmart stanmart merged commit 0ec0935 into main Dec 4, 2024
23 of 24 checks passed
@stanmart stanmart deleted the fix-422 branch December 4, 2024 17:12
@MarcAntoineSchmidtQC MarcAntoineSchmidtQC mentioned this pull request Dec 5, 2024
1 task
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