Skip to content

Expose indentation suppressor from SmartIndenter #4757

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

Merged
merged 12 commits into from
Dec 9, 2015

Conversation

saschanaz
Copy link
Contributor

Separated from #4609.

shouldIndentChildNode function already have indentation suppressing feature, but currently auto-formatter disables it to get delta value and implements another suppressor in computeIndentation function.

This PR split the suppressor from shouldIndentChildNode and expose it as a separate function so that formatter can use it without implementing its own one.

The function will help suppress indentation of:

Note: shouldInheritParentIndentation and its internal nodeWillIndentChild receives TextRangeWithKind to check more conditions other than syntax kind.

@@ -282,19 +282,19 @@ namespace ts.formatting {
*/
function getOwnOrInheritedDelta(n: Node, options: FormatCodeOptions, sourceFile: SourceFile): number {
let previousLine = Constants.Unknown;
let childKind = SyntaxKind.Unknown;
let child: Node = null;
Copy link
Member

Choose a reason for hiding this comment

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

Use undefined, not null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saschanaz saschanaz changed the title Add shouldInheritParentIndentation function to SmartIndenter Expose indentation suppressor from SmartIndenter Sep 14, 2015
@@ -503,7 +490,7 @@ namespace ts.formatting {
indentation -= options.IndentSize;
}

if (SmartIndenter.shouldIndentChildNode(node.kind, SyntaxKind.Unknown)) {
if (SmartIndenter.shouldIndentChildNode(node, null)) {
Copy link
Member

Choose a reason for hiding this comment

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

undefined not null

@RyanCavanaugh
Copy link
Member

Ping @vladima

@@ -493,9 +480,9 @@ namespace ts.formatting {
}
},
getIndentation: () => indentation,
getDelta: () => delta,
getDelta: (child) => (delta && SmartIndenter.shouldInheritParentIndentation(node, child)) ? 0 : delta,
Copy link
Contributor

Choose a reason for hiding this comment

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

to me condition SI.shouldInheritParentIndentation() ? 0 : delta look more clear and it is doing the same thing.

  • if delta is not-null-or-zero then body of getDelta boils down to SI.shouldInheritParentIndentation() ? 0 : delta
  • if delta zero then getDelta evaluates to just delta which will mean the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it's doing the same thing. I was to pass the SI check when delta value is already zero but I agree that it looks less clear. Do you want me to fix this?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

}
},
getIndentation: () => indentation,
getDelta: () => delta,
getDelta: (child) => getEffectiveDelta(delta, child),
Copy link
Member

Choose a reason for hiding this comment

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

remove parens around child

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

}
switch (parent) {

export function nodeWillIndentChild(parent: TextRangeWithKind, child: TextRangeWithKind, indentByDefault: boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be marked as /@internal/

@mhegazy
Copy link
Contributor

mhegazy commented Dec 9, 2015

sorry for the delay. Can you add the @internal annotation in your next PR.

mhegazy added a commit that referenced this pull request Dec 9, 2015
Expose indentation suppressor from SmartIndenter
@mhegazy mhegazy merged commit 88a8345 into microsoft:master Dec 9, 2015
@saschanaz saschanaz deleted the indentSuppressor branch December 9, 2015 14:15
saschanaz added a commit to saschanaz/TypeScript that referenced this pull request May 26, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants