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

Changed IO.read_line to return String rather than String? #4921

Merged
merged 1 commit into from
Sep 5, 2017

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Sep 3, 2017

Fixes #4920

@RX14
Copy link
Member

RX14 commented Sep 3, 2017

Just an entirely offtopic comment: I don't think forwarding argument using *args, **options is a good pattern because it makes the documentation suck. Perhaps we should try to remove this all over the stdlib?

@bew
Copy link
Contributor

bew commented Sep 3, 2017

@RX14 I don't agree, I think that forwarding arguments is good, what needs a fix is the documentation generation.
Maybe the crystal doc could detect the arguments forwarding? Or we could add a mention about it in the docs, like :arg_forwarding: gets, to mark the method as forwarding its arguments to all versions of gets, and allow crystal doc to generate the proper documentation?

@asterite
Copy link
Member

asterite commented Sep 5, 2017

@RX14 I also agree with you regarding the ugly docs, but gets has several overloads.

@jhass jhass merged commit 78ced25 into crystal-lang:master Sep 5, 2017
@jhass
Copy link
Member

jhass commented Sep 5, 2017

Thanks!

@oprypin
Copy link
Member

oprypin commented Sep 5, 2017

@jhass Please note that you seem to be the only one who creates merge commits when merging pull requests. Maybe worth switching to the squash/rebase kind of merge in the UI, just for consistency's sake.

@jhass
Copy link
Member

jhass commented Sep 5, 2017

Oh sorry, I didn't notice we switched to that here, will make sure in the future. Personally I like the merge commit for a pull request though, as it keeps an easy reference to it.

@RX14
Copy link
Member

RX14 commented Sep 6, 2017

@jhass Github's UI will usually show the PR a commit arrived in regardless of how it was merged. If you click on the commit it shows the PR number to the right of the current branch on the left hand side.

@jhass
Copy link
Member

jhass commented Sep 6, 2017

Yes, if you're on Github, which is not always the case ☺

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

6 participants