Rewrite scanner service #91

mrmlnc · 2019-11-18T19:13:25Z

What is the purpose of this pull request?

Update the scanner service.

What changes did you make? (Give an overview)

Rectification of recursion into a loop to improve readability and maintainability. Previously, there was complex and nested recursion here.
Correct processing of partial files. Previously, we do not check the existence of a partial file.
Apparently, we're scanning better now.
Previously, we scan all files after each rename — now only those that have changed.

octref

One thing I just realized was that my original fix for #31 depends on the fact that your scanner would reach into node_modules if an import is explicitly specified.

Do you want to add that functionality back? Otherwise one integration test would be broken.
I think this shouldn't cause much performance issue. For SCSS files in node_modules, scan once and mark the cache frozen. For future scans you can just use the cached results from first time parsing.

octref · 2019-11-18T23:04:29Z

package.json

@@ -53,11 +53,6 @@
 					"default": true,
 					"description": "Allows scan imported files."
 				},
-				"scss.scanImportedFilesDepth": {


I like removal of this option. It doesn't make much sense to me.

You probably want to update README as well.

mrmlnc · 2019-11-19T06:02:52Z

Of course, I will return this functionality.

mrmlnc · 2019-11-19T06:03:47Z

src/server.ts

-	return doScanner(workspaceRoot, cache, settings).then(symbols => {
-		return invalidateCacheStorage(cache, symbols);
-	});
+	return scannerService.scan(files);


We must disable recursive scanning here, because we only check changed files.

mrmlnc · 2019-11-21T20:19:08Z

@octref, please, take a look at this pull request.

src/services/scanner.ts

mrmlnc · 2019-11-21T20:36:02Z

src/server.ts

-	const symbols = await doScanner(workspaceRoot, cache, settings);
-
-	invalidateCacheStorage(cache, symbols);
+	return scannerService.scan(files, /* recursive */ false);


Most likely here we need to always use recursive mode, because a new import may appear in the document. However, this may slow down intellisense.

Right now the cache term is not valid, because we use cache as storage.

octref · 2019-11-22T19:08:51Z

I had a rough look. The change looks good!
I suggest publishing a new version soon and see if user reports any bug.

mrmlnc · 2019-11-22T19:31:48Z

Thanks, Pine!

Yeap, I want to publish the new version on this weekend. With bug fix for #92 (comment)

mrmlnc · 2019-11-22T21:18:57Z

src/providers/completion.ts

 			const isImplicitlyImport = isImplicitly(symbols.document, documentPath, documentImports);
+			const fsPath = getDocumentPath(documentPath, isImplicitlyImport ? symbols.filepath : symbols.document);


Now we use as a reference to the document the real path to the file on the file system.

octref · 2019-11-22T22:04:41Z

One thing we could do is to remove duplicates at the end, right before returning completion, what do you think?

mrmlnc · 2019-11-22T22:13:49Z

Yes, we can do it.

The duplicates in the screenshot above looks like a bug. I will investigate this soon.

mrmlnc · 2019-11-23T10:13:16Z

UPD. This is a bug inside the scss-symbols-parser package.

[                                                                   
  { name: '$caret-width', value: '123', offset: 22 },                // variable
  { name: '$caret-width', value: 'solid transparent', offset: 91 },  // variable inside mixin
  { name: '$caret-width', value: 'solid', offset: 139 },             // variable inside mixin
  { name: '$caret-width', value: 'solid transparent', offset: 173 }  // variable inside mixin
]

mrmlnc mentioned this pull request Nov 18, 2019

Does not open files that start with _ #90

Closed

octref requested changes Nov 18, 2019

View reviewed changes

mrmlnc commented Nov 19, 2019

View reviewed changes

octref mentioned this pull request Nov 19, 2019

scannerExclude depth #55

Closed

mrmlnc force-pushed the rewrite_scanner branch 7 times, most recently from af3fb53 to 94be979 Compare November 21, 2019 19:54

mrmlnc added 9 commits November 21, 2019 22:54

refactor(server): use Async/Await instead of promises chaining

8e366fd

refactor: delete the "scanImportedFilesDepth" option

7d91f0d

refactor(scanner): rectification of recursion into a loop

2fb42ed

build: remove unused dependencies

df54291

refactor: remove unused "ctime" property

fffa1a8

refactor: remove unused "changeActiveDocument" handler

da4ce92

test(e2e): use correct title for test

fb96093

test(e2e): increase sleep time for scanner

e8cccfc

build: target TS to ES2017

166b7df

mrmlnc force-pushed the rewrite_scanner branch from 94be979 to 369c1b6 Compare November 21, 2019 20:13

mrmlnc requested a review from octref November 21, 2019 20:18

mrmlnc force-pushed the rewrite_scanner branch from 369c1b6 to 2326886 Compare November 21, 2019 20:18

mrmlnc commented Nov 21, 2019

View reviewed changes

src/services/scanner.ts Outdated Show resolved Hide resolved

mrmlnc marked this pull request as ready for review November 21, 2019 20:33

mrmlnc commented Nov 21, 2019

View reviewed changes

refactor: introduce StorageService instead of CacheService

2f28f62

Right now the cache term is not valid, because we use cache as storage.

octref approved these changes Nov 22, 2019

View reviewed changes

mrmlnc mentioned this pull request Nov 22, 2019

Roadmap to 1.0.0 #29

Open

12 tasks

mrmlnc force-pushed the rewrite_scanner branch from 3788844 to ca11679 Compare November 22, 2019 21:13

mrmlnc commented Nov 22, 2019

View reviewed changes

mrmlnc added 3 commits November 23, 2019 00:24

fix: use the real path to the file instead of document

39c07e3

Now we use as a reference to the document the real path to the file on the file system.

fix(server): correctly handle added imports

6a5652a

feat(complection): use file path for implicitly complection

5ac7253

mrmlnc force-pushed the rewrite_scanner branch from ca11679 to 5ac7253 Compare November 22, 2019 21:24

mrmlnc merged commit 857b89d into master Nov 22, 2019

mrmlnc deleted the rewrite_scanner branch November 22, 2019 21:37

mrmlnc mentioned this pull request Nov 23, 2019

Fix mixin parsing without parameters mrmlnc/scss-symbols-parser#5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite scanner service #91

Rewrite scanner service #91

mrmlnc commented Nov 18, 2019 •

edited

Loading

octref left a comment

octref Nov 18, 2019

mrmlnc commented Nov 19, 2019

mrmlnc Nov 19, 2019 •

edited

Loading

mrmlnc commented Nov 21, 2019

mrmlnc Nov 21, 2019 •

edited

Loading

octref commented Nov 22, 2019

mrmlnc commented Nov 22, 2019 •

edited

Loading

mrmlnc Nov 22, 2019

octref commented Nov 22, 2019

mrmlnc commented Nov 22, 2019

mrmlnc commented Nov 23, 2019

		const isImplicitlyImport = isImplicitly(symbols.document, documentPath, documentImports);
		const fsPath = getDocumentPath(documentPath, isImplicitlyImport ? symbols.filepath : symbols.document);

Rewrite scanner service #91

Rewrite scanner service #91

Conversation

mrmlnc commented Nov 18, 2019 • edited Loading

What is the purpose of this pull request?

What changes did you make? (Give an overview)

octref left a comment

Choose a reason for hiding this comment

octref Nov 18, 2019

Choose a reason for hiding this comment

mrmlnc commented Nov 19, 2019

mrmlnc Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

mrmlnc commented Nov 21, 2019

mrmlnc Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

octref commented Nov 22, 2019

mrmlnc commented Nov 22, 2019 • edited Loading

mrmlnc Nov 22, 2019

Choose a reason for hiding this comment

octref commented Nov 22, 2019

mrmlnc commented Nov 22, 2019

mrmlnc commented Nov 23, 2019

mrmlnc commented Nov 18, 2019 •

edited

Loading

mrmlnc Nov 19, 2019 •

edited

Loading

mrmlnc Nov 21, 2019 •

edited

Loading

mrmlnc commented Nov 22, 2019 •

edited

Loading