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

Disabled automatic casting to strings #1215

Merged
merged 7 commits into from
Feb 14, 2023
Merged

Conversation

Raalsky
Copy link
Contributor

@Raalsky Raalsky commented Feb 13, 2023

Before submitting checklist

  • Did you update the CHANGELOG? (not for test updates, internal changes/refactors or CI/CD setup)
  • Did you asked the docs owner to review all the updated user-facing interfaces?

@Raalsky Raalsky changed the title Implicit stringify casting Disabled automatic casting to strings Feb 13, 2023
@Raalsky Raalsky requested a review from normandy7 February 13, 2023 14:08
@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Base: 71.36% // Head: 71.32% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (aef3e54) compared to base (4c2682b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##           dev/1.0.0    #1215      +/-   ##
=============================================
- Coverage      71.36%   71.32%   -0.04%     
=============================================
  Files            284      284              
  Lines          13656    13640      -16     
=============================================
- Hits            9745     9729      -16     
  Misses          3911     3911              
Flag Coverage Δ
unit-pull-request 71.32% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/neptune/utils.py 100.00% <ø> (ø)
src/neptune/handler.py 90.12% <100.00%> (+0.25%) ⬆️
src/neptune/integrations/python_logger.py 100.00% <100.00%> (ø)
src/neptune/types/atoms/string.py 89.47% <100.00%> (-1.44%) ⬇️
src/neptune/types/series/string_series.py 85.18% <100.00%> (-1.03%) ⬇️
src/neptune/types/type_casting.py 94.28% <100.00%> (+2.28%) ⬆️
src/neptune/internal/utils/__init__.py 77.41% <0.00%> (-3.23%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

" `stringify_unsupported(collection)` for collections and dictionaries."
" For details, see https://docs.neptune.ai/setup/neptune-client_1-0_release_changes"
)
elif from_stringify_value:
return StringSeries(values=values)
else:
raise TypeError("Value of unsupported type List[{}]".format(type(sample_val)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@normandy7 after 1.0.0 this will raise TypeError if anyone will try to assign an unsupported type (including all that were converted to string in 0.x.x and anything else that were unsupported in prior versions as well). Maybe we would like to improve it anyhow? This also applies to log and extend & append methods and could be specialized to the called method (append and extend are combined).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Raalsky Yes, I think we can use a similar message as before. I think it might be best to create a docs page for this error and how to work around it, then we can simply add a link.

Would something like this work?

            warn_once(
                message="The type of the object you're logging is not supported by"
                " Neptune. To log the object as a string, use `str(object)` or"
                " `stringify_unsupported(collection)` for collections and dictionaries."
                " For details, see https://docs.neptune.ai/help/value_of_unsupported_type"
            )

I can open a separate PR with this if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This couldn't be a warning. Probably a new exception. I'll take care of it later today 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Raalsky Raalsky merged commit 9f6f90d into dev/1.0.0 Feb 14, 2023
@Raalsky Raalsky deleted the rj/implicit-string-casting branch February 14, 2023 11:11
Raalsky added a commit that referenced this pull request Feb 14, 2023
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