-
Notifications
You must be signed in to change notification settings - Fork 58
added support for __str__, __repr__ and _repr_html_ #1490
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
base: main
Are you sure you want to change the base?
Conversation
|
Wow this is looking great @jayendra13 !! |
|
Don't worry about the Rust failures, that's because you don't have permissions to run tests that require secrets. But you do need to run Also, it would be great to have some minimal testing |
|
Yes, tests are the next things, I am going to work on, created a draft PR to know weather the efforts are in correct directions or not and I am covering the necessary classes or not. Would love to hear any feedbacks. |
|
Amended the commit to handle the rust linting failures. |
|
This looks great @jayendra13 !! Feel free to flag as ready for review. We'll have to wait a few days until @TomNicholas has some time to review, he has more context on this topic than I do. But we'll probably merge soon. Thank you! |
|
Will work on test cases here soon, if we want them in the same PR. |
|
Same PR is preferred @jayendra13 . We don't need to be very thorough for this, just a basic test or two. Thanks again! |
f6ec2ac to
4ac53cc
Compare
|
Added simple regex and string matching based python test cases for three classes
with only InMemory storage use-case. |
|
rebasing will get rid of the dependency check error |
|
Rebased. |
Improves Python repr for core classes
Add repr, str, repr_html to Repository, Session, IcechunkStore
Add display helpers in icechunk/src/display.rs
Fixes Nicer python reprs (string and HTML) #1208