Implement VS Code Formatter #535

theRoughCode · 2020-07-27T21:31:50Z

This PR implements the skeleton for the VS Code formatter. We have 2 basic rules for now:

Strip away any whitespace to the left of namespace. For example,
namespace Microsoft.Quantum {} -> namespace Microsoft.Quantum {}.
Format operation definitions. For example:
operation Foo(q:Qubit, n :Int) : Bool -> operation Foo (q : Qubit, n : Int) : Bool

theRoughCode · 2020-07-27T21:37:05Z

cc: @amrondonp, @swosti99

Co-authored-by: Raphael Koh <raph.koh@gmail.com>

Integrate space after if, with tests to formatter branch

…qsharp-compiler into raphael/vsc-formatter

swernli

A few comments and questions, but overall I think this is a great start! Exciting to see the progress!

src/VSCodeExtension/src/extension.ts

swernli · 2020-07-27T21:45:48Z

src/VSCodeExtension/src/formatter/operation-rules.ts

+ * `operation Foo (q : Qubit, n : Int) : Bool`
+ */
+const argsRule = (code: string): string => {
+    const operationMatcher: RegExp = /operation\s*(\w+)\s*\((.*)\)\s*:\s*(\w+)/g;


This is great! Turns out that functions follow the same spacing rules, so you could have the same argsRule implementation for them. Maybe that means later all of this except the literal operation could be moved to a standard utility and both operation-rules.ts and a future function-rules.ts could refer to it? Alternatively, operation-rules.ts could be renamed to be more general and have both operation and function related rules in them, since they are almost identical.

What's a generic umbrella term for operations and functions?

That's a good question, and I don't think there really is one. But if the intent of the file is for checking operation and function declarations, maybe a decent name would be declaration-rules.ts? Then it could also handle user-defined type declarations (newtype as documented here). That file name change can be made in following PRs, no need to gate this PR on the name change.

FYI, we've generally used "callable" to mean "function or operation."

src/VSCodeExtension/src/formatter/index.d.ts

swernli · 2020-07-28T01:52:17Z

Apologies for the delay on the review; I thought I had hit submit but had it sitting open in a browser instead. Great work so far!

swernli · 2020-07-28T01:54:30Z

Looks like there was a failure in the testing of the VSCode extension. Is it possible something in these changes is showing up as a unit test when it shouldn't?
https://dev.azure.com/ms-quantum-public/Microsoft%20Quantum%20(public)/_build/results?buildId=12399&view=logs&j=aa540828-7369-5cca-9a86-6547727f16ff&t=5c10f11e-357e-5f9b-e70d-e2e6470f089e&l=1055

swernli · 2020-07-28T02:01:03Z

Actually, looks like quite a few commits were added between my original review and now. I'm going to take a look back through the files again. Thanks for adding testing!

build/test.ps1

swernli · 2020-07-28T06:24:15Z

src/VSCodeExtension/src/tests/unit/control-structure.test.ts

+  describe("space after if rule", () => {
+    it("adds space after if statement", () => {
+      const code = "if(1 == 1){H(qs[0]);}";
+      const expectedCode = "if (1 == 1){H(qs[0]);}";


Some of these test cases will need to be updated once other rules are implemented (such as requiring a space before and after open curly brace, ie if (1 == 1) { H(qs[0]); }. But that's ok to leave this as-is for now and expect those updates to be done by later rule authors.

cc: @amrondonp

The intention of this rule is only to handle the space after the if statement, We obviously need integration tests that test several rules at once but this specific rule only is concerned about the spacing after the if

swernli

You'll need to make some build script changes to ensure these tests run as expected (and only when expected) in the pipeline.

cgranade · 2020-07-28T15:44:32Z

src/VSCodeExtension/src/formatter/format-document.ts

+    const formattedLines: vscode.TextEdit[] = lines
+        .map((line) => {
+            const formattedLine: string = formatter(line.text, rules);
+            return formattedLine !== line.text


Does this constrain all formatting rules to work one line at a time?

Yup. I'm not sure how to match the whole document at once. Will definitely have to change this to handle multi-line matching.

we could just go lines.join(""); counldn't we?

It would be a bit more involved than that. We need to return vscode.TextEdit[] instead of just the final formatted string.

src/VSCodeExtension/src/formatter/rules/control-structure.ts

cgranade · 2020-07-28T15:47:03Z

src/VSCodeExtension/src/formatter/rules/control-structure.ts

+ * more examples in the unit tests
+ */
+export const spaceAfterIf: FormatRule = (code: string): string => {
+    return code.replace(/(^| +|;|})if[ \n]*/, "$1if ");


Using the * operator could make the regex match on zero instances of [ \n], such that variable names starting with if could be affected. Do you want + here to match [1, ∞) copies of [ \n]?

Suggested change

return code.replace(/(^| +|;|})if[ \n]*/, "$1if ");

return code.replace(/(^| +|;|})if[ \n]+/, "$1if ");

We also want to match none in the case of if(...) -> if (...). I would suggest the following

Suggested change

return code.replace(/(^| +|;|})if[ \n]*/, "$1if ");

return code.replace(/(^| +|;|})if[ |\n]*\(/, "$1if ");

cc: @amrondonp who wrote this

Good suggestion, @theRoughCode thanks!

cgranade · 2020-07-28T15:48:11Z

src/VSCodeExtension/src/formatter/rules/indent.ts

+ * `namespace Foo {}`
+ */
+export const namespaceRule: FormatRule = (code: string) => {
+    const namespaceMatcher: RegExp = /^\s*namespace\s*(\w+)\s*(\S|\S.*\S)\s*$/g;


Does \w+ catch dotted names like Microsoft.Quantum.Arrays?

Good catch. What do you think of this:

/^\s*namespace\s+(\w+(?:\.\w+)*)\s*({|{.*})\s*/$

Please make a test for every edge case you encounter

src/VSCodeExtension/src/formatter/rules/operation.ts

cgranade · 2020-07-28T15:50:18Z

src/VSCodeExtension/src/tests/unit/control-structure.test.ts

@@ -0,0 +1,98 @@
+import "mocha";


I'd suggest a unit test to ensure that if occuring in the beginning, middle, or end of an identifier name doesn't get replaced.

cc: @amrondonp

Sure, Will do

src/VSCodeExtension/src/tests/unit/indent.test.ts

Raphael Koh and others added 8 commits July 27, 2020 15:25

Add Q# formatter

1bdc527

if rules started implementation

3f51ebb

multispaces

dc51f3a

does not change if it is a function call

5dc45c1

with precceding code

72c6407

make text more explicit

f7a4579

Add more tests

87c0a26

Restructure formatters

a985a07

theRoughCode requested a review from swernli July 27, 2020 21:31

theRoughCode self-assigned this Jul 27, 2020

theRoughCode requested a review from ricardo-espinoza July 27, 2020 21:36

Raphael Koh and others added 14 commits July 27, 2020 17:41

Remove hello world test

c8c26a3

Remove Operation.qs

daa3b6d

erase space

c75337c

Merge branch 'raphael/vsc-formatter' into anrondon/vsc-formatter-rules

778d530

Integrate and some little refactoring

626e61f

formatting source code

b822454

Update src/VSCodeExtension/src/formatter/rules/indent.ts

2c75e65

Co-authored-by: Raphael Koh <raph.koh@gmail.com>

changes as suggested

55477d9

Add end lines

befa975

Improve comment

8a7f172

Merge pull request #536 from amrondonp/anrondon/vsc-formatter-rules

548450e

Integrate space after if, with tests to formatter branch

Merge branch 'feature/qslint-hackathon' into raphael/vsc-formatter

c6531b4

Fix test script and add to build pipeline

7b12d2a

Merge branch 'raphael/vsc-formatter' of https://github.com/microsoft/…

f67f94d

…qsharp-compiler into raphael/vsc-formatter

swernli approved these changes Jul 28, 2020

View reviewed changes

swernli self-requested a review July 28, 2020 02:00

swernli reviewed Jul 28, 2020

View reviewed changes

build/test.ps1 Show resolved Hide resolved

swernli reviewed Jul 28, 2020

View reviewed changes

build/test.ps1 Show resolved Hide resolved

swernli reviewed Jul 28, 2020

View reviewed changes

build/test.ps1 Outdated Show resolved Hide resolved

swernli reviewed Jul 28, 2020

View reviewed changes

swernli suggested changes Jul 28, 2020

View reviewed changes

Raphael Koh added 5 commits July 28, 2020 10:45

Fix build pipeline

0d2de3e

Fix indent

59cd426

Restructure test

5ed7447

Add tests for namespace rule

d43d5af

Add tests for operation args rule

d71b4ae

theRoughCode requested a review from swernli July 28, 2020 15:28

swernli approved these changes Jul 28, 2020

View reviewed changes

swernli merged commit cbbd929 into feature/qslint-hackathon Jul 28, 2020

cgranade reviewed Jul 28, 2020

View reviewed changes

theRoughCode mentioned this pull request Jul 28, 2020

Minor fixes and match functions in args formatting rule #537

Merged

theRoughCode deleted the raphael/vsc-formatter branch July 28, 2020 17:25

	return code.replace(/(^\| +\|;\|})if[ \n]*/, "$1if ");
	return code.replace(/(^\| +\|;\|})if[ \n]+/, "$1if ");

	return code.replace(/(^\| +\|;\|})if[ \n]*/, "$1if ");
	return code.replace(/(^\| +\|;\|})if[ \|\n]*\(/, "$1if ");

Implement VS Code Formatter #535

Implement VS Code Formatter #535

Uh oh!

Conversation

theRoughCode commented Jul 27, 2020

Uh oh!

theRoughCode commented Jul 27, 2020

Uh oh!

swernli left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

swernli commented Jul 28, 2020

Uh oh!

swernli commented Jul 28, 2020

Uh oh!

swernli commented Jul 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amrondonp Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swernli 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!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amrondonp Jul 28, 2020 •

edited

Loading