Optional configuration for `multiple imports per line` and strictly `one import per line`. #50

shobhitg · 2018-02-06T06:08:22Z

As per buehler/typescript-hero/issues/379, there should be an option
multiLineWrapMethod, where a user can choose between:
ONE_IMPORT_PER_LINE
OR
MULTIPLE_IMPORTS_PER_LINE

As per buehler/typescript-hero/issues/379, there should be an option `multiLineWrapMethod`, where a user can choose between: `ONE_IMPORT_PER_LINE` OR `MULTIPLE_IMPORTS_PER_LINE`

shobhitg · 2018-02-06T06:49:48Z

This change will provide code generation support from typescript-parser.
I will make another change in typescript-hero to use this facility.

But I am not sure how to push it in since I have another pending change request.

shobhitg · 2018-02-06T06:52:21Z

Once typescript-hero consumes this change, this it will also fix buehler/typescript-hero/issues/358

buehler

Cool idea! And many many thanks for your work. I reviewed it, and I'd suggest some small changes as documented. After that, we're good to go.

buehler · 2018-02-06T06:54:43Z

package.json

@@ -45,7 +46,8 @@
    "typedoc": "^0.9.0"
  },
  "dependencies": {
-    "lodash": "^4.17.4",
+    "lodash": "^4.17.5",


so if you want to use lodash-es instead of lodash (which is fine by me, since I didn't knew it existed) you can remove the other one right? or are both needed?

Both are needed, I only had to add these because I was facing errors in running npm run develop. Right now lodash imports are untouched. At some point I would change those imports to use lodash-es. With that I tihnk we can eventually get treeshaking benefit.

buehler · 2018-02-06T06:59:45Z

src/code-generators/TypescriptGenerationOptions.ts

+     * @type {'MULTIPLE_IMPORTS_PER_LINE' | 'ONE_IMPORT_PER_LINE'}
+     * @memberof TypescriptGenerationOptions
+     */
+    multiLineWrapMethod: 'ONE_IMPORT_PER_LINE' | 'MULTIPLE_IMPORTS_PER_LINE';


I'd use a string enum for this particular case:
This way, it should be exported and is easy extendable (and more convenient to use in an IDE)

export enum MultiLineImportRule { onePerLine = 'onePerLine', multiplePerLine = 'multiplePerLine', }

With this method, we can implement this feature as a normal config in typescript hero and use the enum directly (as string value)

Sounds good.

buehler · 2018-02-06T07:00:41Z

src/code-generators/typescript-generators/namedImport.ts

-        getImportSpecifiers(imp, spaceBraces),
-        lib,
-    );
+    let retVal:string = '';


missing space in code formatting

And please use the whole name returnValue or generatedImport or something like that. I'm not a big fan of abbreviations since we're not coding C or something ;-)

buehler · 2018-02-06T07:02:40Z

src/code-generators/typescript-generators/namedImport.ts


-    if (importString.length > multiLineWrapThreshold) {
+    if (specifiers.length > multiLineWrapThreshold) {


This actually changes the behaviour of the multi line wrap threshold. before, the generated string was checked for his length. Now only the specifiers.

Or did I misread something?

It should behave like before, or we need to introduce a BREAKING CHANGE

I actually intended this change, because I thought that it is the right thing to do, didn't realize that it could be considered a Breaking change.

Actually, I will rever that part for now. Its small thing to change later if think that it should be done.

buehler · 2018-02-06T07:03:54Z

src/code-generators/typescript-generators/namedImport.ts

+        const sortedImportSpecifiers: SymbolSpecifier[] = imp.specifiers.sort(specifierSort);
+        let importSpecifierStrings: string = '';
+
+        if (multiLineWrapMethod === 'MULTIPLE_IMPORTS_PER_LINE') {


Please use the string enum, as mentioned above

buehler · 2018-02-06T07:07:19Z

config/tsconfig.base.json

@@ -7,7 +7,7 @@
        "outDir": "../build",
        "rootDir": "../src",
        "declaration": true,
-        "sourceMap": false,
+        "sourceMap": true,


Please not in the base config. The tsconfig.jsonconfiguration does specify sourceMap: true. The base should not change that since the production config does not overwrite the setting again.

-> false

buehler · 2018-02-06T07:08:59Z

To consume the change, we will release the parser and then we can update it in typescript hero. Afterwards the settings needs to be coded in TSH

shobhitg · 2018-02-06T17:27:46Z

I think there are some inefficiencies in this file in general. There is some logic we are doing more an once.
I will take some time to re-think and refactor the code.
Also, I will keep buehler/typescript-hero/issues/397 in mind.

buehler · 2018-02-06T17:37:00Z

Thank you so much 🙇
I'm currently implementing tests for typescript hero.

shobhitg · 2018-02-11T00:10:16Z

I have made some changes as per what I think are the best set of options.

But I am not very good with tests, I just updated them using the test script.

@buehler Please review and let me know if anything needs improvement, and also see if you can help with tests.

shobhitg · 2018-02-11T00:46:14Z

Just FIY, the option oneImportPerLineOnlyAfterThreshold should be default in ts-hero as per legacy behavior.

shobhitg · 2018-02-11T19:55:34Z

Option strictlyOneImportPerLine will take care of buehler/typescript-hero/issues/351

shobhitg · 2018-02-23T17:12:58Z

@buehler Can you please review this? Once we are settled with this change, then I would need to make a parallel ts-hero change to make it work with this commit, and once that is reviewed, both can go in together.

Shobhit Gupta added 2 commits February 5, 2018 22:02

Optional configuration for multiple imports per line

24d465b

As per buehler/typescript-hero/issues/379, there should be an option `multiLineWrapMethod`, where a user can choose between: `ONE_IMPORT_PER_LINE` OR `MULTIPLE_IMPORTS_PER_LINE`

Fix Lint and unit tests.

c1e84da

buehler requested changes Feb 6, 2018

View reviewed changes

Merge branch 'develop' into develop

8a1d5e5

Shobhit Gupta added 3 commits February 10, 2018 15:51

Restructuring import organization.

aed9a25

Set sourceMaps to false again as per review.

59e1013

Updated tests.

6dda58a

shobhitg changed the title ~~Optional configuration for multiple imports per line~~ Optional configuration for multiple imports per line and strictly one import per line. Feb 11, 2018

buehler approved these changes Feb 26, 2018

View reviewed changes

buehler merged commit f8f72f4 into buehler:develop Feb 26, 2018


		if (importString.length > multiLineWrapThreshold) {
		if (specifiers.length > multiLineWrapThreshold) {

Optional configuration for multiple imports per line and strictly one import per line. #50

Optional configuration for multiple imports per line and strictly one import per line. #50

Uh oh!

Conversation

shobhitg commented Feb 6, 2018

Uh oh!

shobhitg commented Feb 6, 2018

Uh oh!

shobhitg commented Feb 6, 2018

Uh oh!

buehler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buehler commented Feb 6, 2018

Uh oh!

shobhitg commented Feb 6, 2018

Uh oh!

buehler commented Feb 6, 2018

Uh oh!

shobhitg commented Feb 11, 2018

Uh oh!

shobhitg commented Feb 11, 2018

Uh oh!

shobhitg commented Feb 11, 2018

Uh oh!

shobhitg commented Feb 23, 2018

Uh oh!

Uh oh!

Optional configuration for `multiple imports per line` and strictly `one import per line`. #50

Optional configuration for `multiple imports per line` and strictly `one import per line`. #50