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

fix issue 17925 - Mention the old body keyword #1929

Merged
merged 6 commits into from Nov 20, 2017
Merged

fix issue 17925 - Mention the old body keyword #1929

merged 6 commits into from Nov 20, 2017

Conversation

ghost
Copy link

@ghost ghost commented Nov 20, 2017

New comers might be confused by the specs, not matching the D code they can read here and there so a note about the syntax explains the situation.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @bbasile! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
17925 [Contract Programming]

On the long term, <code>body</code> could be completely unsupported but for now it's still allowed,
as a keyword in this context and as an identifier in the function body
although <code>do</code> must be prefered.
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR. Please consider this wordsmithing:

The actual function body starts with do.
In the past, body was used, and could still be encountered in old code bases.
In the long term, body may be deprecated, but for now it's still allowed
as a keyword in this context and as an identifier in the function body, although do is preferred.

@JinShil
Copy link
Contributor

JinShil commented Nov 20, 2017

Output doesn't look right. Maybe missing a parentheses or parentheses in the wrong place.

image

@ghost
Copy link
Author

ghost commented Nov 20, 2017

Yeah i need to add a paragraph but that's not ideal. It looks like because of the style the paragraph will have its own number.

@ghost
Copy link
Author

ghost commented Nov 20, 2017

I think this is good now but actually this is for the stable branch. The problem was also that the change was not in the changelog.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

I would add a link to the accepted DIP:

https://github.com/dlang/DIPs/blob/master/DIPs/DIP1003.md


$(P The actual function body starts with $(D do).
In the past, $(D body) was used, and could still be encountered in old code bases.
In the long term, $(D body) may be deprecated, but for now it's still allowed
Copy link
Member

Choose a reason for hiding this comment

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

Remove 'still'

Copy link
Member

Choose a reason for hiding this comment

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

Why don't you state that it will be deprecated. After all, the regarding DIP has been approved.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, but the problem is not the compiler change, the problem is the documentation that has been brutally changed without a changelog entry, that's the point of the bugzilla report. Even in phobos you still use body.

@wilzbach
Copy link
Member

Even in phobos you still use body.

Excellent point -> dlang/phobos#5869

@PetarKirov
Copy link
Member

PetarKirov commented Nov 20, 2017

@wilzbach I'm not sure, but I don't think DScanner has support for the do keyword (in place of body) yet.

Nevermind, that was meant as a reply to dlang/phobos#5869

@PetarKirov
Copy link
Member

ZombineDev removed the auto-merge label 35 seconds ago

I just want to check the final website output before merging.

@ghost
Copy link
Author

ghost commented Nov 20, 2017

yeah, let's wait, i'm not comfy with ddoc, i'm not sure if the changes will be okay.

@ghost
Copy link
Author

ghost commented Nov 20, 2017

I think we're good now ?

@dlang-bot dlang-bot merged commit 3ccba5e into dlang:master Nov 20, 2017
@PetarKirov
Copy link
Member

I think we're good now ?

Yes, thanks!

@ghost ghost deleted the issue-17925 branch April 23, 2018 13:34
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