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 docs of big module overloads #6336

Merged
merged 1 commit into from
Jul 15, 2018

Conversation

laginha87
Copy link
Contributor

Fix for #6226 by adding code examples to the docs that import the big module.

It was also mentioned in the issue to improve the error message in case of overflow, but from what I looked at the code it seems like there is no simple way to add that without changing the code significantly.
https://github.com/crystal-lang/crystal/blob/e806c95/src/string.cr#L407

@Sija
Copy link
Contributor

Sija commented Jul 5, 2018

Convention is to add require statement (in the examples) only once, in the module docs. See Colorize for instance.

@asterite
Copy link
Member

asterite commented Jul 5, 2018

@Sija These are methods added to Int and Float, it makes sense to show that require "big" is needed for those methods to work.

@asterite
Copy link
Member

asterite commented Jul 5, 2018

@laginha87 Please rebase this PR's branch against master, CI is failing because of things that are already fixed in master.

@laginha87 laginha87 force-pushed the laginha87-improve-big-docs branch from 714de7c to d7fec28 Compare July 5, 2018 16:20
@@ -298,6 +304,12 @@ module Math
{frac, exp}
end

# Returns the sqrt of a `BigInt`.
Copy link

Choose a reason for hiding this comment

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

Should be BigFloat

@RX14 RX14 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:docs labels Jul 7, 2018
Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Well, apart from what @BenDietze pointed out.

@laginha87 laginha87 force-pushed the laginha87-improve-big-docs branch from d7fec28 to fbcaf58 Compare July 10, 2018 08:25
@laginha87
Copy link
Contributor Author

@RX14, @BenDietze thanks for looking into the PR, I made the requested change and rebased on master again, should be good to go.

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @laginha87 👍

@sdogruyol sdogruyol merged commit 3b3bf3f into crystal-lang:master Jul 15, 2018
@sdogruyol sdogruyol added this to the Next milestone Jul 15, 2018
@RX14 RX14 modified the milestones: Next, 0.26.0 Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants