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

Add missing require statements for the API #7564

Conversation

Maroo-b
Copy link
Contributor

@Maroo-b Maroo-b commented Mar 20, 2019

The purpose of this PR is to add missing require statements in examples, it addresses this issue

I went through the current docs (0.27.2) and check all the provided examples.

Issues

Iterator example:
link

array_of_iters = [[1], [2, 3], [4, 5, 6]]
iter = Iterator(Int32).chain array_of_iters
ter.next # => 1
iter.next # => 2
iter.next # => 3
iter.next # => 4

returns this error:

instantiating 'Iterator::ChainsAll(Array(Int32), Int32)#next()'
src/iterator.cr:207: undefined method 'next' for Array(Int32)

@Maroo-b Maroo-b force-pushed the 7487_add_missing_require_statements branch from 9c0370d to 29cb212 Compare March 20, 2019 13:12
@Sija
Copy link
Contributor

Sija commented Mar 20, 2019

IMHO require statements in docs should be added only at the module level. Repeating those for every example seems repetitive and unnecessary.

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

There are some inconsistencies.

I like that the samples can be run directly by copy/pasting them 👍 .

src/big/big_int.cr Show resolved Hide resolved
src/big/big_rational.cr Show resolved Hide resolved
src/big/big_rational.cr Show resolved Hide resolved
@Maroo-b Maroo-b force-pushed the 7487_add_missing_require_statements branch from 29cb212 to 92ac496 Compare March 20, 2019 16:02
@Maroo-b
Copy link
Contributor Author

Maroo-b commented Mar 20, 2019

@bcardiff thanks for the quick review, I updated the PR to cover all the API and fix the style inconsistency.

@Maroo-b Maroo-b changed the title Add missing require statements for the API (WIP) Add missing require statements for the API Mar 20, 2019
@Maroo-b Maroo-b force-pushed the 7487_add_missing_require_statements branch from 92ac496 to ba4b234 Compare March 20, 2019 20:59
@Sija
Copy link
Contributor

Sija commented Mar 21, 2019

That's a serious bloat within the docs, whereas it pretty easy to just check main module doc comment for a needed require...

@bcardiff
Copy link
Member

@Sija having self-contained examples is good. Even at the cost of 2 duplicate lines IMO.
Maybe for libs where more context is needed that does not apply. But at least for the stdlib is something I think we should aim.

@straight-shoota
Copy link
Member

I've been questioning the noise this generates, but having them is better than not having them.

And it could perhaps in the future be an option to optimize the display of API docs. It's easy to recognize repeating require statements and make them fade or disappear or whatever. It doesn't work the other way around.

@bcardiff
Copy link
Member

@Maroo-b I think we are ready to merge this as is.
The issue you notice is fixed in #7569
I think there is no conflict between both PR so it's better to merge this, then the other and then if wanted the [M-Z] range can be tackled.

ok?

@Maroo-b
Copy link
Contributor Author

Maroo-b commented Mar 22, 2019

@bcardiff I guess it can be merged, also I already covered [A-Z] (which means every doc page for 0.27.2 version.

@Maroo-b
Copy link
Contributor Author

Maroo-b commented Mar 22, 2019

@bcardiff @straight-shoota I would like to contribute more to Crystal in my free time, is there a tag or a board for highest priority issues or tasks that I can work on?

@straight-shoota straight-shoota merged commit 5282fdf into crystal-lang:master Mar 22, 2019
@straight-shoota
Copy link
Member

Thank you very much @Maroo-b! I'm looking forward for additional contributions from you.

There is no real priority list. Important issues are usually ones tagged with the next milestone but they're often more complex bugs (or features that are already worked). Another starting point are the community: labels, for example community:to-implement. Apart from that, just pick whatever you're interested in. You can browse the list of open issues and take what you like ;)

@straight-shoota straight-shoota added this to the 0.28.0 milestone Mar 22, 2019
@Maroo-b
Copy link
Contributor Author

Maroo-b commented Mar 22, 2019

@straight-shoota thank you for your quick feedback!

Sure, I'll check the labeled issues :)

urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Mar 26, 2019
* upstream/master:
  fix example codes (2019-03) (crystal-lang#7569)
  Change bsearch's `/ 2` to `>> 1` for faster performance (crystal-lang#7531)
  Don't include private constants in the docs  (crystal-lang#7575)
  Add missing `require` statements for the API (A-N) (crystal-lang#7564)
  Implement Int#{leading,trailing}_zeros_count (crystal-lang#7520)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants