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

Update data-classes.md to avoid making assumption when choosing data class and class #3603

Closed
wants to merge 7 commits into from

Conversation

hieuwu
Copy link

@hieuwu hieuwu commented Jun 4, 2023

🖼 Summary

Currently at the beginning, this page is making assumption that every classes might need some utility functions that comes with data class . However, in some codebase not every class should be marked as data class if the purpose of using these objects does not require any utility functions.
It's good to make a clear understanding when choose using data class and normal class.
In some libraries, they use only class to form a constraint of data to make sure every clients that want to use data from these classes, should only read properties from them and no utility function are required from these classes. In this case, using class is good enough

💡 Update

Add a reminder of usage class or data class should be based on each situation

@sarahhaggarty sarahhaggarty self-assigned this Jul 13, 2023
@sarahhaggarty sarahhaggarty self-requested a review July 13, 2023 08:20
@sarahhaggarty
Copy link
Collaborator

Hi @hieuwu, thanks for your suggestion! After thinking long and hard about this I decided that instead of adding a clarification later like you suggest, it would be better to improve the introductory paragraph. I made this change as part of #3711 Now it should be clear that data classes are a special type of class that come with additional member functions. Thanks for sharing your thoughts with us!

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.

2 participants