This is an archive of the discontinued LLVM Phabricator instance.

clang-format: [JS] merge import lines.
ClosedPublic

Authored by mprobst on Apr 14 2021, 4:44 AM.

Details

Summary

Multiple lines importing from the same URL can be merged:

import {X} from 'a';
import {Y} from 'a';

Merge to:

import {X, Y} from 'a';

This change implements this merge operation. It takes care not to merge in
various corner case situations (default imports, star imports).

Diff Detail

Event Timeline

mprobst requested review of this revision.Apr 14 2021, 4:44 AM
mprobst created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 4:44 AM
h-joo added a subscriber: h-joo.Apr 14 2021, 5:33 AM
h-joo added inline comments.
clang/lib/Format/SortJavaScriptImports.cpp
154

Would it be better if this were a named function?

164

nit : It?

clang/unittests/Format/SortImportsTestJS.cpp
333

I'm a bit confused on what this is trying to test. Could you explain?

mprobst updated this revision to Diff 337456.Apr 14 2021, 8:04 AM

address review comments.

mprobst marked 3 inline comments as done.Apr 14 2021, 8:06 AM
mprobst added inline comments.
clang/lib/Format/SortJavaScriptImports.cpp
92

FYI I restructured the algorithm a bit here.

clang/unittests/Format/SortImportsTestJS.cpp
333

This makes sure we don't collapse import * as foo imports with import {A} style imports.

h-joo accepted this revision.Apr 14 2021, 8:11 AM

LGTM

This revision is now accepted and ready to land.Apr 14 2021, 8:11 AM
This revision was landed with ongoing or failed builds.Apr 14 2021, 8:20 AM
This revision was automatically updated to reflect the committed changes.
mprobst marked an inline comment as done.
jgorbe added a subscriber: jgorbe.Apr 21 2021, 1:28 PM

Hi, one of our tests is failing because of this patch and it looks like an actual bug. clang-format now turns this file:

import './a';
import {bar} from './a';

into this:

barimport './a';