Skip to content

Dont disturb whitespace preceding comments - #1166

Merged
basoundr merged 2 commits into
dotnet:masterfrom
basoundr:fix1151WhitespaceBeforeComment
Mar 11, 2015
Merged

Dont disturb whitespace preceding comments#1166
basoundr merged 2 commits into
dotnet:masterfrom
basoundr:fix1151WhitespaceBeforeComment

Conversation

@basoundr

Copy link
Copy Markdown
Contributor

Fix #1151 : Dont disturb the whitespaces preceding the comments which
are also not part of indentation to be converted to tabs if 'Keep Tabs'
option is set.

Fix dotnet#1151 : Dont disturb the whitespaces preceding the comments which
are also not part of indentation to be converted to tabs if 'Keep Tabs'
option is set.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With this change the (completely untested 😦) UseTabOnlyForIndentation option has no effect anymore. The idea of that option is to use Tabs for the base indentation of the statement, and then spaces for alignment of consecutive lines after that initial set of tabs.

Perhaps we should either:

  1. Remove the option, since it's untested and not exposed through the UI.
  2. Add it back, put some tests in place, and use a different method to calculate alignment spaces and the spaces between tokens.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's just remove the option.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ooh, for what it's worth, that option sounds awesome. But if I had to choose, I'd rather have #1151 fixed than the option (for now).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have removed the option. Given that we dont have any test for this option and no more using this option in the code, it is ideal to have this option removed.

Issue #1176 has been opened to track the addition of the option with precise intent and testcases

Remove UseTabOnlyForIndentation since this is a public option & there
are no testcases for this option. With the changes the option is no
longer used making it obsolete. Fix the broken testcase which was
asserting the undesired behavior
@basoundr
basoundr force-pushed the fix1151WhitespaceBeforeComment branch from 5474397 to f879029 Compare March 10, 2015 21:23
@basoundr

Copy link
Copy Markdown
Contributor Author

@Pilchie @heejaechang Please take a look at the final changes. This is a RC bug that needs to be fixed by tomorrow.

Other potential reviewers : @jasonmalinowski @dpoeschl @rchande @balajikris @brettfo

@Pilchie

Pilchie commented Mar 11, 2015

Copy link
Copy Markdown
Member

👍

basoundr added a commit that referenced this pull request Mar 11, 2015
Dont disturb whitespace preceding comments
@basoundr
basoundr merged commit c6fd5a4 into dotnet:master Mar 11, 2015
@basoundr
basoundr deleted the fix1151WhitespaceBeforeComment branch March 11, 2015 16:45
dibarbet pushed a commit that referenced this pull request Aug 18, 2026
Fix compliation errors in Testing README sample custom verifier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Format Document munges whitespace preceding // single line comments

5 participants