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 docs for Int#downto #12468

Merged
merged 7 commits into from
Oct 31, 2022
Merged

Add docs for Int#downto #12468

merged 7 commits into from
Oct 31, 2022

Conversation

yb66
Copy link
Contributor

@yb66 yb66 commented Sep 10, 2022

Update the iterator's related var name for consistency.

Did this because it only had bare signature for docs, and the to argument reads badly.

@straight-shoota
Copy link
Member

Could you leave the parameter renaming out of this PR and just add the docs for now? I think that change might need some more discussion first.

@HertzDevil
Copy link
Contributor

You can probably make to an external parameter name and limit the internal one to get the best of both worlds.

@yb66
Copy link
Contributor Author

yb66 commented Sep 14, 2022

The last commit doesn't touch the code but I think it highlights why I feel the name change is better. Compare this:

Get an iterator for counting down from self to to

with this:

Get an iterator for counting down from self to limit

Read it out loud and it's really obvious (to my ear).

You can probably make to an external parameter name and limit the internal one to get the best of both worlds.

Method arguments gives the 2nd reason for using named parameters as…

The second use case is making a method parameter more readable inside a method body

but I'm more concerned with how the documentation reads, I'd rather that limit were the external argument.

From the issue

It's not a huge improvement

Perhaps I'm bikeshedding but my view is that code can be made to sound more like English but will always hit a limit somewhere and read like code (that's why it's called code, after all) but the documentation is English and thus any possibility to make it flow better is a win.

This should also be discussed together with Int#upto

Agreed. How about we keep this docs only, I'll push up docs only for upto as I now notice that has no docs, and that'll close this and the linked issue and I'll open a new one to keep the discussion in one place?

I just noticed that trailing semicolon, I'll fix that now too.

@HertzDevil
Copy link
Contributor

HertzDevil commented Sep 14, 2022

But giving different external and internal parameter names doesn't imply that the docs must use the former. For instance, Steppable does this:

def step(*, to limit = nil, by step, exclusive : Bool = false, &) : Nil
Iterates from self to limit incrementing by the amount of step on each iteration. If exclusive is true, limit is excluded from the iteration.

@yb66
Copy link
Contributor Author

yb66 commented Sep 15, 2022

I would always prefer renaming local vars over changing an API (changing a message signature). The scope of the instance vars is limited by being part of a very short interface, so it's also not a dangerous change IMO.

I now see that the current naming convention extends throughout the file and I'm getting confused with my own "fixup" commits so I'll stick to add the docs only and leave the code for another day.

src/int.cr Outdated Show resolved Hide resolved
src/int.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota changed the title Added docs for both downto, updated arg name for style reasons. #12467 Add docs for Int#downto Sep 15, 2022
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 @yb66 👍

@straight-shoota straight-shoota added this to the 1.7.0 milestone Oct 29, 2022
@straight-shoota straight-shoota merged commit 36bd91e into crystal-lang:master Oct 31, 2022
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.

Adding docs for Integer#downto
4 participants