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

convert _default_params to dataclass #237

Merged
merged 12 commits into from
Oct 17, 2024
Merged

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Oct 16, 2024

improve readability and reduce eventual typo errors while manipulating keys

with IDE it is now feasible to track usage
also suggesting to rename default_params to global_params as it is better fit

@Borda Borda requested a review from shaypal5 as a code owner October 16, 2024 11:32
@Borda Borda force-pushed the dataclass/_default_params branch from 24baf76 to edd1b0f Compare October 16, 2024 15:41
@shaypal5

This comment was marked as off-topic.

@shaypal5
Copy link
Collaborator

Ok, I let GitHub merge the pre-commit.ci fix into this branch, and it now works, but fails. :)

@Borda
Copy link
Contributor Author

Borda commented Oct 17, 2024

Ok, I let GitHub merge the pre-commit.ci fix into this branch, and it now works, but fails. :)

@shaypal5 done and resolved last linting issue

Copy link
Collaborator

@shaypal5 shaypal5 left a comment

Choose a reason for hiding this comment

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

Great! I love the new data class, and the deprecation warning. Let's remember to bump the major part of the version number when we push out the breaking change of delete the set_deafult_params method and any of its friends.

@shaypal5 shaypal5 merged commit 9dd89b0 into master Oct 17, 2024
33 checks passed
@shaypal5 shaypal5 deleted the dataclass/_default_params branch October 17, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants