Make parsing and formatting utility method naming more accurate and consistent #925
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…I patterns * Also makes them more consistent with RespRead/WriteUtils naming * Revamp xmldoc comments. * Includes some adjustments in consumption of these methods, no behavioral changes.
…ead/WriteUtils. * This makes it the assumptions of the method much more visible to callers.
* NumUtils.TryReadDouble/Int64 has no (sane) situatioin it would throw on here.
…tency. Add exception throwing xmldoc details to TryRead methods
|
I think this is a good and sensible code clean-up, we should review and get this merged. |
TalZaccai
approved these changes
Jan 23, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have been going back and forth for long time when would be best time to try introduce these changes, so here goes.
This PR makes major naming changes in the "low-level" utility classes:
NumUtilsandRespRead/WriteUtils. The naming changes are largely mechanical:Try-prefix to all parsing/formatting methods that returnboolindicating whether reading/writing was successful or not.i. This matches the .NET API design convention I think majority .NET developers are/should be familiar with. I'm talking about the distinction between
int.Parseandint.TryParse. It makes it clear to the developer they need to think about the case where the method can fail "silently" if the return value is ignored. Generally theseTrymethods are also the more performant ones, they handle the majorirty that they don't usually throw, and as such incur any exception handling costs.(U)Longto(U)Int64. This one is very much preference thing, but after I saw method named asRead64Int, I went ahead with this naming change proposal. (This is also the convention in BCL)NumUtils.Method naming is largely very subjective and hard, but I think these changes are warranted as I believe they improve readability and reduce the mental overhead when working with the parsing/formatting parts of the code-base. I also think that more accurate method naming conventions especially improve code-review process.
I'm very happy and prefer to wait with this PR to minimize any all the conflicts this will have with older PRs still in progress.