-
Notifications
You must be signed in to change notification settings - Fork 399
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
[FEATURE] add __repr__
to argilla.settings._resource.SettingsProperties
#5380
Comments
Cool project!! I would like to work on this if it is okay. I am pretty inexperienced but this should be self-explanatory. I could check for other classes too but let me start with this. 🙂 |
Hi @phershbe, that would be very welcome. You can tak e a look here to see how you can contribute but feel free to ask for help when needed. https://docs.argilla.io/latest/community/contributor/ |
@phershbe are you still interested in tackling this? |
@DavidBerenstein Sorry about this. Yeah I am, I spent quite a bit of time and went through the code base and checked out the |
hi @davidberenstein1957 , can I take up this? |
Hi @davidberenstein1957 : Can you please help me with replicating the scenarios. I was able to run all the unit tests without any failure. So, if I understand correctly, I will have to add tests to check the correctness of the code change. |
hi @davidberenstein1957, Request you to pls confirm if the below behavior is the expected behaviour for the below unit test.
Without fix
With fix
|
That looks great! |
Thanks @davidberenstein1957 , raising PR now. |
Is your feature request related to a problem? Please describe.
These are used for error messaging but don't show the relevant information.
Describe the solution you'd like
I would like to see it as normal printed settings for the dataset.
Describe alternatives you've considered
NA.
Additional context
There might be other classes that need a
__repr__
.The text was updated successfully, but these errors were encountered: