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

Improve fibonacci code in Getting Started page #2901

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

MitchellGale
Copy link
Contributor

The Fibonacci demo on the getting started page does not account for overflows. This fix improves it by throwing an error upon overflow (fib 94 or above for uint64).

Old:

What Fibonacci number would you like to know: 
1
Fibonacci(1) = 1
What Fibonacci number would you like to know: 
93
Fibonacci(93) = 12200160415121876738
What Fibonacci number would you like to know: 
94
Fibonacci(94) = 1293530146158671551
What Fibonacci number would you like to know: 
95
Fibonacci(95) = 13493690561280548289
What Fibonacci number would you like to know: 
100
Fibonacci(100) = 3736710778780434371

New:

What Fibonacci number would you like to know: 
1
Fibonacci(1) = 1
What Fibonacci number would you like to know: 
93
Fibonacci(93) = 12200160415121876738
What Fibonacci number would you like to know: 
94
Fibonacci(94): unsigned integer overflow
What Fibonacci number would you like to know: 
95
Fibonacci(95): unsigned integer overflow
What Fibonacci number would you like to know: 
100
Fibonacci(100): unsigned integer overflow

@MitchellGale MitchellGale requested review from a team as code owners June 22, 2023 06:24
@pellared
Copy link
Member

pellared commented Jun 22, 2023

Also please check https://github.com/open-telemetry/opentelemetry.io/actions/runs/5342310912/jobs/9684106429?pr=2901

EDIT: I guess you should use "tabs" 😬

@svrnm svrnm merged commit d3be3a6 into open-telemetry:main Jun 26, 2023
8 checks passed
@MrAlias
Copy link
Contributor

MrAlias commented Jun 26, 2023

This should not have been merged. The error was intentional. It is addressed in a later section: https://github.com/MitchellGale/opentelemetry.io/blob/806d4094e6a1fcf6ce681643dad0f86b534853e2/content/en/docs/instrumentation/go/getting-started.md#bonus-errors

Having this error is important as it helps instruct users how to handle errors with their traces.

@chalin
Copy link
Contributor

chalin commented Jun 26, 2023

@MrAlias @svrnm et al.: To avoid this from happening again, I'd like to suggest that we add a comment (if only a footnote) to mention that the error is intentional, and will be addressed later. WDYT?

@MrAlias
Copy link
Contributor

MrAlias commented Jun 26, 2023

@MrAlias @svrnm et al.: To avoid this from happening again, I'd like to suggest that we add a comment (if only a footnote) to mention that the error is intentional, and will be addressed later. WDYT?

SGTM 👍

@svrnm
Copy link
Member

svrnm commented Jun 27, 2023

@MrAlias @svrnm et al.: To avoid this from happening again, I'd like to suggest that we add a comment (if only a footnote) to mention that the error is intentional, and will be addressed later. WDYT?

yes, let's do that.

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.

None yet

5 participants