Skip to content

Added list, set, and tuple support for log_configs(), added tuple support for tags #67

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Nov 5, 2024
Merged

Added list, set, and tuple support for log_configs(), added tuple support for tags #67

merged 10 commits into from
Nov 5, 2024

Conversation

SiddhantSadangi
Copy link
Member

No description provided.

@SiddhantSadangi
Copy link
Member Author

cc: @neptune-ai/tech-writers for documentation

@SiddhantSadangi SiddhantSadangi self-assigned this Oct 31, 2024
@SiddhantSadangi SiddhantSadangi changed the title Converting non-string series elements to strings for logging Added list, set, and tuple support for log_configs(), added tuple support for add_tags() Oct 31, 2024
@SiddhantSadangi SiddhantSadangi changed the title Added list, set, and tuple support for log_configs(), added tuple support for add_tags() Added list, set, and tuple support for log_configs(), added tuple support for tags Oct 31, 2024
@SiddhantSadangi SiddhantSadangi added bug Something isn't working enhancement New feature or request labels Oct 31, 2024
kgodlewski
kgodlewski requested changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@kgodlewski kgodlewski left a comment

Choose a reason for hiding this comment

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

I would limit this PR to only adding typehints for tuples. Supporting nontrivial types for configs requires deeper consideration. Adding it now by casting to str could lock us in for longer if users start depending on this behaviour.

@SiddhantSadangi SiddhantSadangi force-pushed the ss/log_configs_series_support branch from 9fe3abb to 23b16df Compare November 5, 2024 14:14
@kgodlewski
Copy link
Contributor

Please merge with main again

@kgodlewski
Copy link
Contributor

Something is wrong, as the changes include files that already exist. I think you should revert the last commit and rebase onto main, preferably git rebase -i and removing the extra commits

@SiddhantSadangi
Revert "Merge commit '85bb57efbe10ad5eec4caa74ff3a6b33ca4f626c' into …
08c9400 
…ss/log_configs_series_support"

This reverts commit 2f2226d.
@SiddhantSadangi SiddhantSadangi merged commit ae56869 into main Nov 5, 2024
4 checks passed
@SiddhantSadangi SiddhantSadangi deleted the ss/log_configs_series_support branch November 5, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgodlewski kgodlewski kgodlewski approved these changes

@michalsosn michalsosn Awaiting requested review from michalsosn

@PatrykGala PatrykGala Awaiting requested review from PatrykGala

@normandy7 normandy7 Awaiting requested review from normandy7

Assignees

@SiddhantSadangi SiddhantSadangi

Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SiddhantSadangi @kgodlewski