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

Nested attr supporting keys #631

Merged
merged 7 commits into from
Oct 2, 2024
Merged

Nested attr supporting keys #631

merged 7 commits into from
Oct 2, 2024

Conversation

Isaac-Flath
Copy link
Contributor

Added support for keys in nested_attr

@jph00

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jph00 jph00 added the enhancement New feature or request label Oct 2, 2024
@jph00
Copy link
Member

jph00 commented Oct 2, 2024

Can you look at the failing CI @Isaac-Flath ? Lemme know if you need help fixing it.

@jph00
Copy link
Member

jph00 commented Oct 2, 2024

That was fast :)

@jph00
Copy link
Member

jph00 commented Oct 2, 2024

Got another CI issue @Isaac-Flath !

@Isaac-Flath
Copy link
Contributor Author

Isaac-Flath commented Oct 2, 2024

Argh, another issue. I am going to sleep on this and fix it in the morning. I will get the fastai testing figured out for local tests so I can test these integration tests that are failing before pushing the next attempt.

@Isaac-Flath
Copy link
Contributor Author

@jph00 All tests pass now, but it feels like there should be a better way. I tried looking for a __getattr__ method and tried checking for types, but wasn't able to get something that worked reliably in all cases without doing the try and catching the exception if it happens.

@jph00
Copy link
Member

jph00 commented Oct 2, 2024

@Isaac-Flath i think you want the hasattr() function

@Isaac-Flath
Copy link
Contributor Author

Isaac-Flath commented Oct 2, 2024

I used it in some of the previous versions that were failing the integration tests, and tried several things using it and kept failing test so got rid of it. But I just added it back and it seems to work on local fastcore and fastai tests, so I must have been doing something different wrong.

Anyway - much better. Thank you!

@jph00 jph00 merged commit cc75f59 into fastai:master Oct 2, 2024
@jph00
Copy link
Member

jph00 commented Oct 2, 2024

Thanks @Isaac-Flath . In your other PR that uses this, please update settings.ini to require this version of fastcore , and at-mention when that's ready, and I'll release both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants