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

fix: return "0x00" for normalize(0) #306

Closed
wants to merge 2 commits into from

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Apr 25, 2023

The normalize function returns 0x${number} for all numbers except 0, for which it returns undefined. This changes it to return 0x00 for 0.

if (!input) {
return undefined;
}

if (typeof input === 'number') {
if (input < 0) {
Copy link
Contributor Author

@legobeat legobeat Apr 25, 2023

Choose a reason for hiding this comment

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

@adonesky1 @kumavis This stood out to me. Do you recall the context on why this returns 0x for all negative numbers instead of throwing like it does on other invalid input?
1e4f176#diff-39b2554fd18da165b59a6351b1aafff3714e2a80c1435f2de9706355b4d32351R111

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I guess the intention is to throw on negative numbers.

// TODO: Add validation to disallow negative integers.

I can address that in a separate change (as it's a breaking one) after this one has been merged and released.

@legobeat legobeat marked this pull request as ready for review April 25, 2023 02:32
@legobeat legobeat requested a review from a team as a code owner April 25, 2023 02:32
@legobeat legobeat changed the title fix: return "0x0" for normalize(0) fix: return "0x00" for normalize(0) Apr 25, 2023
kumavis
kumavis previously approved these changes Apr 26, 2023
Copy link
Member

@kumavis kumavis left a comment

Choose a reason for hiding this comment

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

good

this is a breaking change

@legobeat legobeat requested review from Mrtenz and Gudahtt April 27, 2023 11:11
Gudahtt
Gudahtt previously approved these changes Apr 27, 2023
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@legobeat
Copy link
Contributor Author

legobeat commented May 1, 2023

Not certain if this should be merged and released before or after #273.

@mikesposito
Copy link
Member

I didn't notice this PR before opening this one: #315
@legobeat I am thinking if maybe #315 could solve the problem you are trying to fix?

@mcmire
Copy link

mcmire commented Jun 23, 2023

@mikesposito @legobeat This seems complementary to #315. There would be conflicts whether we merge one first or the other, but I don't see a reason why we couldn't have both. Do you?

Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Makes sense.

@mikesposito
Copy link
Member

mikesposito commented Jun 26, 2023

@mcmire We can definitely have both, I was just pointing out that after #315 has been merged, the early function termination does not affect 0 as input anymore, so from this PR we could also just add the test

@mikesposito mikesposito mentioned this pull request Jun 26, 2023
@Gudahtt
Copy link
Member

Gudahtt commented Jun 29, 2023

I don't think the functional change made in this PR is relevant anymore. The change in #315 is functionally equivalent in the case where zero is passed in, though broader in that it also affects the case where an empty string is passed in.

Agreed that the test would be useful to include though!

@Gudahtt
Copy link
Member

Gudahtt commented Jun 29, 2023

I have created a separate PR to add just the test: #317

@Gudahtt
Copy link
Member

Gudahtt commented Jun 29, 2023

Superseded by #315 and #317

@Gudahtt Gudahtt closed this Jun 29, 2023
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.

5 participants