Skip to content

Conversation

@julijane
Copy link
Contributor

While it is only a two liner, it feels inconsistent to have a DecodeBase64 and a DecodeHex function, but not a DecodeBytes function. This PR adds that.

I'm wondering if the tests for DecodeBase64, DecodeHex and DecodeBytes should actually have a single shared table of test cases?

@julijane julijane requested a review from blahspam as a code owner March 15, 2025 12:44
@blahspam
Copy link
Collaborator

I think you're right. Merging all DecodeXXX tests makes a lot of sense. Mind tackling that in this PR?

@julijane
Copy link
Contributor Author

Sure, I can do that. Question: How do you think it should be done, i'm now not sure?

Option a.) Have each testcase have the input as base64, as hex and as a slice of bytes.
Option b.) Just have one, for example base64, and convert it in the test to the required format.

I was thinking of Option b.) as Option a.) seems extensive, but then again it would make it a bit questionable to have tests for more than one of the functions in the first place (but it counts as test coverage I guess). Probably doing Option a.) is the best bet against the encoding/decoding breaking (but do we expect to have go's base64 or hex en/decoding to ever break?)

So what do you prefer?

@blahspam
Copy link
Collaborator

Yeah I think option b is the better of the two. The test case can then detect whether its a base64 or hex string and decode and validate accordingly.

@julijane julijane force-pushed the juli/decode-from-bytes branch from 0ffa30a to 98679a9 Compare March 16, 2025 19:42
@julijane
Copy link
Contributor Author

Well, after I have been thinking a bit more about this (and before you replied), I had now chosen option a.), because it makes this more independent. WDYT?

@julijane julijane force-pushed the juli/decode-from-bytes branch 2 times, most recently from f86cd22 to 73cbd2d Compare March 16, 2025 19:50
@julijane julijane changed the title feat: add DecodeBytes function add DecodeBytes function, refactor tests Mar 16, 2025
@blahspam
Copy link
Collaborator

I'm good with that.

@julijane
Copy link
Contributor Author

Well, then this can be merged :) I first thought there were two more test cases that should be integrated, but that does not work, so I had deleted my comment regarding this again.

@blahspam blahspam force-pushed the juli/decode-from-bytes branch from 73cbd2d to 23231e7 Compare March 17, 2025 15:06
@blahspam blahspam merged commit 86ecb1f into Comcast:main Mar 17, 2025
2 checks passed
@julijane julijane deleted the juli/decode-from-bytes branch March 17, 2025 17:49
@blahspam blahspam added the enhancement New feature or request label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants