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 indent detection when lexing docstrings that contain whitespace-only lines. #2131

Merged
merged 1 commit into from
Aug 4, 2017

Conversation

mfelsche
Copy link
Contributor

@mfelsche mfelsche commented Aug 4, 2017

fixes #2130

@mfelsche mfelsche changed the title [docgen] don't consider lines with only WS for common indent detection lexer: don't consider lines with only WS for common indent detection Aug 4, 2017
if(ws_this_line < ws)
if(!isspace(c)) {
has_non_ws = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the {/} braces for this - the prevailing style in this codebase is to omit braces when the body is just one statement.

if(!isspace(c))
  has_non_ws = true;

Also note that if the braces were to be included (for a multi-line if statement), the prevailing style is to place the opening brace on its own line:

if(!isspace(c))
{
  has_non_ws = true;
  some_other_statement();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, i still struggle to get my editor (and my head) to respect those formatting rules.

@jemc jemc changed the title lexer: don't consider lines with only WS for common indent detection Fix indent detection when lexing docstrings that contain whitespace-only lines. Aug 4, 2017
@jemc jemc added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Aug 4, 2017
@jemc
Copy link
Member

jemc commented Aug 4, 2017

This is ready to merge when CI passes.

I added the changelog label and edited the ticket title to read like a changelog entry, so that the automatic changelog bot can do its work.

Thanks!

@SeanTAllen SeanTAllen merged commit 9bbcaeb into ponylang:master Aug 4, 2017
ponylang-main added a commit that referenced this pull request Aug 4, 2017
@SeanTAllen
Copy link
Member

Thanks @mfelsche

@SeanTAllen
Copy link
Member

one small request for the future @mfelsche the "fixes" is nice ideally information about the "why" would be in the commit message itself rather than just a link to the issue. the best case for the body of that commit would be an explanation of the problem as well as a link to the github issue.

@mfelsche
Copy link
Contributor Author

mfelsche commented Aug 5, 2017

Point taken. I have to admit i was in a great hurry when i pushed and opened the pr. Will take more care on future contribution.

@SeanTAllen
Copy link
Member

thanks @mfelsche

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docgen] docstrings not rendered correctly
3 participants