Enforce code style via linter (part of #446) #448

danyill · 2021-09-17T20:32:37Z

This will eventually close #446 but is draft for now.

Updating dependencies to use standard for lining has resulted in package-lock being rewritten to a new version.

Immediate output is: 3242 problems.
After auto-fixing we get 1269 problems.
Most seem to be spaces and tabs and things which are never used.
I'm happy to go through these.

I guess ideally we would wait until we had no open PRs before doing something like this but this PR just allows us to understand the implications.

WDYT?

danyill · 2021-09-17T20:33:54Z

I had thought it would be good to set up a test framework prior to making "big changes" but the extension does still seem to work at this point. I have no experience as to whether linters are likely to break working code.

ggrossetie · 2021-09-18T10:30:31Z

Updating dependencies to use standard for lining has resulted in package-lock being rewritten to a new version.

The reason package-lock.json contains a lot of changes is because you are using npm <= 6. You should use npm 7 on this project (with Node >= 14).
Currently, the lock file is using version 2 (you can see it in the diff: lockfileVersion: 2).
If you install dependencies with npm <= 6 it will regenerate the lock file using version 1.

I guess ideally we would wait until we had no open PRs before doing something like this

Yes, otherwise we will need to rebase and resolve conflicts.

I had thought it would be good to set up a test framework prior to making "big changes" but the extension does still seem to work at this point.

Indeed that would be good but it will take time until we have a good test overage.

I have no experience as to whether linters are likely to break working code.

I believe that standard --fix is safe unless there's a bug in standard.
Having said that, it's probably a good idea to perform extensive (manual) tests once all lint issues are fixed but before merging this pull request.

danyill · 2021-09-19T11:08:18Z

Something is happening -- and it certainly took some time.

23:03 $ npm run lint

> asciidoctor-vscode@0.0.0 lint
> eslint 'src/**/*.ts' 'src-preview/**/*.ts' --format unix

/home/mulhollandd/Documents/asciidoctor-vscode/src/asciidocExtensions.ts:12:31: 'Thenable' is not defined. [Error/no-undef]
/home/mulhollandd/Documents/asciidoctor-vscode/src/asciidocExtensions.ts:21:30: 'Thenable' is not defined. [Error/no-undef]
/home/mulhollandd/Documents/asciidoctor-vscode/src/asciidocExtensions.ts:51:36: 'Thenable' is not defined. [Error/no-undef]
/home/mulhollandd/Documents/asciidoctor-vscode/src/commands/openDocumentLink.ts:40:30: Expected 'undefined' and instead saw 'void'. [Error/no-void]
/home/mulhollandd/Documents/asciidoctor-vscode/src/commands/openDocumentLink.ts:42:21: Expected 'undefined' and instead saw 'void'. [Error/no-void]
/home/mulhollandd/Documents/asciidoctor-vscode/src/features/workspaceSymbolProvider.ts:13:30: 'Thenable' is not defined. [Error/no-undef]
/home/mulhollandd/Documents/asciidoctor-vscode/src/features/workspaceSymbolProvider.ts:99:47: 'Thenable' is not defined. [Error/no-undef]
/home/mulhollandd/Documents/asciidoctor-vscode/src/features/workspaceSymbolProvider.ts:135:59: 'Thenable' is not defined. [Error/no-undef]
/home/mulhollandd/Documents/asciidoctor-vscode/src/security.ts:23:91: 'Thenable' is not defined. [Error/no-undef]
/home/mulhollandd/Documents/asciidoctor-vscode/src/security.ts:29:57: 'Thenable' is not defined. [Error/no-undef]
/home/mulhollandd/Documents/asciidoctor-vscode/src/security.ts:59:99: 'Thenable' is not defined. [Error/no-undef]
/home/mulhollandd/Documents/asciidoctor-vscode/src/security.ts:72:63: 'Thenable' is not defined. [Error/no-undef]

The Thenable didn't have a type definition. I took one from the Markdown repo and placed in src/typings. But I can't seem to get eslint to recognise it. And I'm a little uncertain about the undefined/void at the moment.

I haven't done any significant testing yet and I would want to spend some time on it because there were quite a few non-automatic changes and I may have introduced errors (particularly around isNullOrUndefined where I decided it shouldn't always need to be written as the or of both expressions but I may not have always chosen correctly).

ggrossetie · 2021-09-21T12:19:28Z

The Thenable didn't have a type definition. I took one from the Markdown repo and placed in src/typings. But I can't seem to get eslint to recognise it.

I think we need to add:

{
  "env": {
    "node": true,
    "es6": true
  }
}

in the .eslintrc.js file

And I'm a little uncertain about the undefined/void at the moment.

I can't understand why we do such things:

Promise.resolve(void 0)
        .then(() => vscode.commands.executeCommand('vscode.open', resource))
        .then(() => void 0)

vscode.commands.executeCommand('vscode.open', resource) is asynchronous and void 0 is a function that returns undefined but we could write:

public async execute(args: OpenDocumentLinkArgs) {
  // ...
  await vscode.commands.executeCommand('vscode.open', resource)
  return
}

That's almost what the Markdown plugin is doing: https://github.com/microsoft/vscode/blob/2f33470e28c1a3440b2c5e0ea9205b1d0a1e6f1b/extensions/markdown-language-features/src/commands/openDocumentLink.ts#L66-L82

ggrossetie · 2021-09-23T07:01:48Z

@danyill I've replaced Thenable by Promise (included in Node typing) and replaced Promise.resolve(void 0) by async/await.

ggrossetie · 2021-09-23T07:05:36Z

I've also enabled GitHub Action CI workflow (lint and compile) on pull requests

danyill · 2021-09-23T10:27:02Z

Thank you, that's marvellous. I'd like to spend an hour or two on the weekend seeing if I can find obvious flaws and then hopefully we can merge a few things.

…iidoctor-vscode into issue-446-code-style

danyill · 2021-09-25T05:39:16Z

ggrossetie · 2021-09-25T09:51:02Z

Check that links work correctly (FAIL they don't but I think there are issues logged about this so I haven't fixed here)

From what I seen in the code (before your changes) it won't work because the regular expressions are designed to work with Markdown syntax (not AsciiDoc syntax).

asciidoctor-vscode/src/features/documentLinkProvider.ts

Lines 53 to 55 in 74bce1d

    
           private readonly linkPattern = /(\[[^\]]*\]\(\s*)((([^\s\(\)]|\(\S*?\))+))\s*(".*?")?\)/g; 
        
           private readonly referenceLinkPattern = /(\[([^\]]+)\]\[\s*?)([^\s\]]*?)\]/g; 
        
           private readonly definitionPattern = /^([\t ]*\[([^\]]+)\]:\s*)(\S+)/gm;

If you're happy with this PR @Mogztter feel free to merge away otherwise I'll be happy to look into any aspect in more detail.

If you can confirm what exactly is not working with links (to make sure that this is related to existing issues) that would be great. Then, I think we can safely merge this pull request.

Great work 🎉

danyill · 2021-09-25T10:08:19Z

Ah yes, those too. I was thinking of links in the WebView which I think used to work...

…

On Sat, 25 Sep 2021, 21:51 Guillaume Grossetie, ***@***.***> wrote: Check that links work correctly (FAIL they don't but I think there are issues logged about this so I haven't fixed here) From what I seen in the code (before your changes) it won't work because the regular expressions are designed to work with Markdown syntax (not AsciiDoc syntax). https://github.com/asciidoctor/asciidoctor-vscode/blob/74bce1d292f36a1672d6df608d1aa4d1e8a85720/src/features/documentLinkProvider.ts#L53-L55 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#448 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFEXX6U3ID252DH4XSRDE3UDWLRDANCNFSM5EIJKHVQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

danyill · 2021-09-25T20:29:54Z

If you can confirm what exactly is not working with links (to make sure that this is related to existing issues) that would be great. Then, I think we can safely merge this pull request.

I've had a look, I think it's unrelated.

Links within the preview were only recently added (see #397 and #400) and worked fine previously. I think the Webview is now handled differently due to upstream changes and file links end up with a prefix of https://file+.vscode-resource.vscode-webview.net/. This doesn't work with the previously link handling strategy so we need to make sure these are handled differently so they are resolved correctly.

asciidoctor-vscode/src-preview/index.ts

Lines 143 to 155 in 74bce1d

    
           	if (passThroughLinkSchemes.some((scheme) => node.href.startsWith(scheme))) { 
        
           		return; 
        
           	} 
        
           	hrefText = node.getAttribute('href'); 
        
           } 
        
           // If original link doesn't look like a url, delegate back to VS Code to resolve 
        
           if (!/^[a-z\-]+:/i.test(hrefText)) { 
        
           	messaging.postMessage('clickLink', { href: hrefText }); 
        
           	event.preventDefault(); 
        
           	event.stopPropagation(); 
        
           	return; 
        
           }

At least half a fix looks something like the following where we make sure we don't pass through file links and then ensure that it is handled by VS code to resolve.

			let hrefText = node.getAttribute('data-href');
			if (!hrefText) {
				// Pass through known schemes
				if (passThroughLinkSchemes.some((scheme) => node.href.startsWith(scheme) && !node.href.startsWith('https://file+.vscode-resource.vscode-webview.net/'))) {
					return;
				}
				hrefText = node.getAttribute('href');
			}

			// If original link doesn't look like a url, delegate back to VS Code to resolve
			if (!/^[a-z\-]+:/i.test(hrefText) || hrefText.startsWith('http://https//file+.vscode-resource.vscode-webview.net')) {
				messaging.postMessage('clickLink', { href: hrefText });
				event.preventDefault();
				event.stopPropagation();
				return;
			}

I'll look into this as part of #435.

ggrossetie · 2021-09-26T07:06:53Z

Perfect, then we are good to go 😀

danyill added 2 commits September 18, 2021 08:13

Enforce code style via linter (part of asciidoctor#446)

1c54549

Add ESLint autofixed files

29b440d

danyill added 5 commits September 18, 2021 15:17

Convert tabs to 2 spaces

6de5bd1

Solve the easy linting issues

443bcd4

Add a few more, ignore linting on folders with 'test'

51a1340

Fix typos, more linter happiness, add rule to disallow semicolons

2a92755

Quote eslint properly, solve some more issues

23527ef

danyill added 3 commits September 19, 2021 22:09

And some more linting goodness

fc0bdd2

Update package-lock.json

6c5206b

Some more linting happiness

bc0d787

ggrossetie mentioned this pull request Sep 22, 2021

Load Highlight.js from the extension #451

Merged

ggrossetie added 2 commits September 23, 2021 08:51

Import Thenable and replace Promise.resolve(void 0) by async/await

131889b

Replace Thenable by Promise

a2ac8dd

ggrossetie marked this pull request as ready for review September 23, 2021 07:01

Run CI on pull requests

4a645ab

danyill added 2 commits September 25, 2021 17:35

Fix null/undefined check conditions for PDF export

97e5d1d

Merge branch 'issue-446-code-style' of https://github.com/danyill/asc…

42b9798

…iidoctor-vscode into issue-446-code-style

ggrossetie merged commit cd477c7 into asciidoctor:master Sep 26, 2021

danyill added this to the 2.8.10 milestone Oct 21, 2021

meznom mentioned this pull request Mar 4, 2022

User setting asciidoc.asciidoctorpdf_command is ignored #532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce code style via linter (part of #446) #448

Enforce code style via linter (part of #446) #448

danyill commented Sep 17, 2021

danyill commented Sep 17, 2021

ggrossetie commented Sep 18, 2021

danyill commented Sep 19, 2021

ggrossetie commented Sep 21, 2021

ggrossetie commented Sep 23, 2021

ggrossetie commented Sep 23, 2021

danyill commented Sep 23, 2021

danyill commented Sep 25, 2021 •

edited by ggrossetie

Loading

ggrossetie commented Sep 25, 2021 •

edited

Loading

danyill commented Sep 25, 2021 via email

danyill commented Sep 25, 2021

ggrossetie commented Sep 26, 2021

Enforce code style via linter (part of #446) #448

Enforce code style via linter (part of #446) #448

Conversation

danyill commented Sep 17, 2021

danyill commented Sep 17, 2021

ggrossetie commented Sep 18, 2021

danyill commented Sep 19, 2021

ggrossetie commented Sep 21, 2021

ggrossetie commented Sep 23, 2021

ggrossetie commented Sep 23, 2021

danyill commented Sep 23, 2021

danyill commented Sep 25, 2021 • edited by ggrossetie Loading

ggrossetie commented Sep 25, 2021 • edited Loading

danyill commented Sep 25, 2021 via email

danyill commented Sep 25, 2021

ggrossetie commented Sep 26, 2021

danyill commented Sep 25, 2021 •

edited by ggrossetie

Loading

ggrossetie commented Sep 25, 2021 •

edited

Loading