Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix named export/import formatting#4609

Merged
mhegazy merged 27 commits into
microsoft:mastermicrosoft/TypeScript:masterfrom
saschanaz:fixExportImportFormattingsaschanaz/TypeScript:fixExportImportFormattingCopy head branch name to clipboard
Jun 27, 2016
Merged

Fix named export/import formatting#4609
mhegazy merged 27 commits into
microsoft:mastermicrosoft/TypeScript:masterfrom
saschanaz:fixExportImportFormattingsaschanaz/TypeScript:fixExportImportFormattingCopy head branch name to clipboard

Conversation

@saschanaz

Copy link
Copy Markdown
Contributor

Fixes #4516, fixes #6924.

@saschanaz saschanaz changed the title Fix export import formatting Fix named export/import formatting Sep 2, 2015
@DanielRosenwasser

Copy link
Copy Markdown
Member

@saschanaz tests/cases/fourslash/formatNamedExportImport.ts seems to test templates, not imports (or exports).

@saschanaz

Copy link
Copy Markdown
Contributor Author

@DanielRosenwasser I just fixed it, sorry! ;)

@mhegazy

mhegazy commented Sep 3, 2015

Copy link
Copy Markdown
Contributor

looks like we have a marge conflict with the other formatting changes.

Comment thread src/services/formatting/rules.ts Outdated

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.

I'm having trouble understanding why these were removed.

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.

@DanielRosenwasser The SpaceAfterAwaitKeyword and SpaceAfterTypeKeyword rules are merged into SpaceAfterCertainKeywords and SpaceAfterCertainTypeScriptKeywords rules respectively.

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.

What about the NoSpaceAfterAwaitKeyword one? Is that just encompassed by SpaceAfterCertainKeywords?

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.

The other NoSpace ones should not exist. I once thought they will work together with Space rules but then found they should only exist when there are some valid formatting options.

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.

I once thought NoSpace rules would first remove those whitespaces and after that Space rules would add one. However, a Space rule can remove excessive whitespaces by itself.

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.

I once thought NoSpace rules would first remove those whitespaces and after that Space rules would add one. However, a Space rule can remove excessive whitespaces by itself.

That's what I thought as well - it does make sense that one doesn't necessarily need the other though.

@DanielRosenwasser

Copy link
Copy Markdown
Member

Looks good to me - @vladima can you take a quick peek?

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.

Not sure why this is needed? Can we just do what is done for all other kinds of blocks, namely:

  • add SyntaxKind.NamedImports, SyntaxKind.NamedExports to the list in shouldIndentChildNode function so its content will be indented
  • update isCompletedNode function to correctly process case when cursor is in incompleted NamesImport / NamedExport node by using hasChildOfKind(n, SyntaxKind.CloseBraceToken, sourceFile);

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.

hasBraceShell function is not to check completeness as ImportClause node can omit its optional braces. I think isCompletedNode function is to check a node has its required children.

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.

@vladima I was wrong and the braces are not optional. I just removed hasBraceShell and added a simpler check.

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.

@saschanaz currently nodeIsCompleted handles cases when code is incomplete i.e.

import {$ // hit enter at $, caret on the next line should be indented
```.

I would expect imports and export to follow the same technique that we already use for all other blocks\block like constructions

saschanaz added a commit to saschanaz/TypeScript that referenced this pull request Sep 9, 2015
@saschanaz saschanaz mentioned this pull request Sep 9, 2015
3 tasks

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.

Can you move this comment down? Also, is it correct?

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.

I later changed it to // NamedImports has its own braces as Block does (so do not give it additional indentation)

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.

@vladima Former hasBraceShell (and current namedBindings check) is needed to prevent unwanted excessive indentation. Consider this code:

import
{
    x as xx
}
from "foo"

import
    * as Foo from "foo"

ImportDeclaration has ImportClause node and then ImportClause node can have two different kind of nodes: NamedImports and NamespaceImport.

NamedImports, as you noted, looks like a block, but NamespaceImport doesn't. If I decide to always indent ImportClause or always not to do it, formatting will look bad:

import
    {
        x as xx
    } // when ImportClause is always indented
from "foo"

import
* as Foo from "foo" // when always not

Thus I added this check.

Anyway I agree that isCompletedNode should be updated. I'll push a change for it.

Comment thread src/services/formatting/formatting.ts Outdated

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.

we don't use nulls - use undefined instead

@vladima

vladima commented Oct 8, 2015

Copy link
Copy Markdown
Contributor

LGTM after comments are addresses

@saschanaz

Copy link
Copy Markdown
Contributor Author

@vladima Can you also review #4757? shouldInheritParentIndentation part of this PR is in fact duplication of that one. I'll remove the duplication after #4757 is merged.

@mhegazy

mhegazy commented Dec 9, 2015

Copy link
Copy Markdown
Contributor

looks good. please address the comments about use of null.

@saschanaz

Copy link
Copy Markdown
Contributor Author

I want to format export/imports and object destructuring (not in this PR) with similar rules but I'm not sure what should I do in this case:

export
{ // Export block in this PR can optionally have a newline before open brace as `if` block can
    x, y
} from "foo"

let
{ // Will anyone want this?
    x, y
} = foo;

@mhegazy

mhegazy commented Dec 9, 2015

Copy link
Copy Markdown
Contributor

👍
@vladima can you take a look.

@saschanaz saschanaz force-pushed the fixExportImportFormatting branch from d8a8843 to a9245c6 Compare February 19, 2016 04:46
@mhegazy

mhegazy commented Jun 27, 2016

Copy link
Copy Markdown
Contributor

thanks @saschanaz. can you also add a condition for #9224 to allow users to opt in/out.

@saschanaz saschanaz deleted the fixExportImportFormatting branch September 20, 2016 23:45
@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.

Format code unindents multi-line import statement Support formatting for named imports/export clauses

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.