-
Notifications
You must be signed in to change notification settings - Fork 286
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
cleanup: const a few tox pointers for groupchat API #2379
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2379 +/- ##
=========================================
+ Coverage 9.98% 9.99% +0.01%
=========================================
Files 141 141
Lines 32530 32530
=========================================
+ Hits 3247 3251 +4
+ Misses 29283 29279 -4
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 0 of 1 approvals obtained
ca5af25
to
cfaa31a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. There's no API break because there's no release with this API in it, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robinlinden pointed out, correctly, that we tend to use const
in the public API to mean deep-immutable. What do you think? Should all Tox parameters be const
everywhere?
cfaa31a
to
576fcfc
Compare
Correct (unless you count client releases)
Is there ever a valid reason for it not to be const? |
If we only make the tox functions const that don't modify things inside the tox state, then it helps client code ensure that if they pass a |
@JFreegman I think the documentation aspect of "const means no changes happen to the object" in a broad sense is useful in the public API. Maybe someday I'll write a linter to enforce something like that, but I'm not sure what the syntax and semantics of that should be. |
Closing this for now. Reopen if you want to discuss reasoning for what "const" should express in the public API. |
This change is