Format implementation assignments, ifs #41

kdorsel · 2020-07-16T18:27:24Z

Initial go ahead with formatting of the implementation. Works with assignments and IF statements.

Format this:

msg.CreateEx(evt, 0);
IF len(str) > 0 AND len(str) > 0 AND len(str) > 0 AND len(str) > 0 AND len(str) > 0 AND len(str) > 0 THEN
	msg.SetJsonAttribute(str);
END_IF

msg.Send(0);

Into this:

msg.CreateEx(evt, 0);
IF len(str) > 0 
	AND len(str) > 0
	AND len(str) > 0
	AND len(str) > 0
	AND len(str) > 0
	AND len(str) > 0
THEN
	msg.SetJsonAttribute(str);
END_IF

msg.Send(0);

I just want to stop here with a working example and go over any major changes to implementation before continuing on this path.

Roald87

Hi,

Sorry for the late reply, I was quite busy lately. Thanks for the first draft!

Some comments/ideas per file.

IfBlockStart

In IfBlockStart it would be good to have a Tokenize method to clean up the Format method a bit. Now there is a lot going on in the format method, where it does both tokenizing and formatting. Maybe use a regex to put each statement as a separate item in a list? When you do this also clean them of all spaces. You could for example put them in a new struct Statement which has three members leftOperand, operand and rightOperand?

Currently you only check if the unformatted code + the indentation is longer than 88, but it can happen that the unformatted code contains double spaces or no spaces. I think it should be a two step process. For example you make a method FormatInSingleLine which formats the if statement as a single line. Then it checks the length of that. if it is longer than 88, it will call FomatOverMultipleLines.

Finally, this

IF len(str) > 0 
	AND len(str) > 0
	AND len(str) > 0
	AND len(str) > 0
	AND len(str) > 0
	AND len(str) > 0
THEN

should be

IF 
	len(str) > 0 
	AND len(str) > 0
	AND len(str) > 0
	AND len(str) > 0
	AND len(str) > 0
	AND len(str) > 0
THEN

ImplementationCode

I think it is a good idea to make a separate class as you did with ImplementationCode. Then it would be a good idea to also make a DeclarationCode class and move all declaration specific code to there from CompositeCode. All the general code will remain in CompositeCode class.

There is a lot of white space fixing in here. I think this isn't be necessary with my suggestions for IfBlockStart.

Roald87 · 2020-07-26T13:55:15Z

src/TcBlack/TcPou.cs

+                    .Tokenize()
+                    .Format(ref indents);
+                string firstLine;
+                if (


Why is this if statement here?

This is something I've started with my own projects. The first line of the implementation is always a comment // or empty. This is because when doing git diffs on the xml it's easier to read if the first code line is not attached to the <![CDATA[. This is separating the TwinCAT markup vs the actual user code.

For example

<Implementation> <ST><![CDATA[ F_CreateHashTableHnd(ADR(table), SIZEOF(table), hTable); ]]></ST> </Implementation>

vs

<Implementation> <ST><![CDATA[F_CreateHashTableHnd(ADR(table), SIZEOF(table), hTable); ]]></ST> </Implementation>

Wouldn't it even better that it always is an empty line? When putting a comment there you have the same issue as with code right?

I prefer either one of the following options:

Always insert an empty line, but not an (empty) comment

Remove all empty lines at the start.

If you choose option 1, then I guess it is also needed for the declaration part.

Finally I prefer if you just add an EmptyLine class at the start when you loop over the lines in the Tokenize method.

Roald87 · 2020-07-26T13:58:37Z

src/TcBlack/VariableAssignment.cs

+        public override string Format(ref uint indents)
+        {
+            TcAssignment assign = Tokenize();
+            return _singleIndent.Repeat(indents) + assign.LeftOperand + " := " + assign.RightOperand;


Please use string interpolation for this https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/interpolated. Also at the other places.

So this becomes $"{_singleIndent.Repeat(indents)}{assign.LeftOperand} := {assign.RightOperand};"$ .

Roald87 · 2020-07-26T15:40:05Z

I've made the line ending and indentation into global variables (#42). This shortens the initialization of the different code types a lot. You can also add the line length as a global variable.

kdorsel · 2020-07-27T14:31:16Z

All sounds good to me, I'll make the changes. One thing I'm unsure about is what to do with multi assignments.

a := b := c := 4;

Are these allowed? Or shall they be broken up into single lines?
The left operand will therefore need to be an array.

…Black into implementation-format

Multi line function calls inside an if don't work

kdorsel · 2020-07-30T20:05:34Z

Ok, now supports fixing multiline function calls (not inside an if though). Also fixed a bunch of other parsing issues.

One major item that needs to be addressed is recursively setting types. This is why function calls inside an IF ... THEN don't work as it treats as plain text currently.

DbAck.execRet3(ADR(qryTool1), ADR(partId), SIZEOF(partId), arg1 := F_UDINT(deviceId), arg2 := F_INT(localGlobals.machiningToolSet[deviceId - 4].insert[1].number), arg3 := F_INT(localGlobals.machiningToolSet[deviceId - 4].insert[1].rotation), )

Becomes

DbAck.execRet3(
	ADR(qryTool1),
	ADR(partId),
	SIZEOF(partId),
	arg1:=F_UDINT(deviceId),
	arg2:=F_INT(localGlobals.machiningToolSet[deviceId-4].insert[1].number),
	arg3:=F_INT(localGlobals.machiningToolSet[deviceId-4].insert[1].rotation),
);

Roald87

One thing I'm unsure about is what to do with multi assignments.

Personally I'm not a fan and would break them up, but Black for Python allows it so TcBlack will also allow it.

Roald87 · 2020-08-11T19:05:16Z

src/TcBlack/TcPou.cs

+                    .Tokenize()
+                    .Format(ref indents);
+                string firstLine;
+                if (


Wouldn't it even better that it always is an empty line? When putting a comment there you have the same issue as with code right?

I prefer either one of the following options:

Always insert an empty line, but not an (empty) comment

Remove all empty lines at the start.

If you choose option 1, then I guess it is also needed for the declaration part.

Finally I prefer if you just add an EmptyLine class at the start when you loop over the lines in the Tokenize method.

Roald87 · 2020-08-11T19:11:24Z

src/TcBlack/UnknownCodeType.cs

@@ -12,7 +12,7 @@ string lineEnding

        public override string Format(ref uint indents)
        {
-            return _unformattedCode;
+            return _singleIndent.Repeat(indents) + _unformattedCode;


Wouldn't this add extra spaces if the original code already has the correct spacing? Since there is no trim method used when the code is added to the CompositeCode.codeLines list.

Roald87 · 2020-08-11T19:28:07Z

src/TcBlack/FunctionStatement.cs

+            );
+        }
+
+        protected string FormatSingle(ref uint indents, TcVariablePassing func, bool multi)


Can you break this up into FormatIntoSingleLine and FormatIntoMultiLine methods? Then you do not need bool mutli and all the if statements in the method. Plus the name conveys better what the method does.

Roald87 · 2020-08-11T19:31:02Z

src/TcBlack/FunctionStatement.cs

+                formated += Global.lineEnding;
+            }
+            foreach(
+                string ea


What does ea stand for? Can you use non-abbreviated variable names?

Roald87 · 2020-08-11T19:31:41Z

src/TcBlack/IfBlockStart.cs

+
+        public struct IfStatement
+        {
+            public IfStatement(


Can you also use non-abbreviated variable names here?

Roald87 · 2020-08-11T19:37:31Z

src/TcBlack/IfBlockStart.cs

+            return formattedCode;
+        }
+
+        public override string Format(ref uint indents)


Can you move the Format method above FormatSingle? Then the code gets more detailed as you read along plus it is the natural order: first you tokenize, then format, which consists of the more detailed FormatSingle and FormatMultiple methods.

Roald87 · 2020-08-14T18:42:54Z

General comment. It would be a good idea to break up this PR into multiple ones. Like one for function calls, one for if statements, etc. It is becoming quite large now and difficult to review. Also it would be good to have unit tests from the start.

Format implementation assignments, ifs

8f2179e

kdorsel changed the title ~~WIP Format implementation assignments, ifs~~ WIP: Format implementation assignments, ifs Jul 16, 2020

kdorsel marked this pull request as draft July 16, 2020 18:29

kdorsel changed the title ~~WIP: Format implementation assignments, ifs~~ Format implementation assignments, ifs Jul 16, 2020

Roald87 requested changes Jul 26, 2020

View reviewed changes

kdorsel added 5 commits July 27, 2020 12:23

Merge branch 'master' into implementation-format

580047e

Merge branch 'master' into implementation-format

e757373

Merge branch 'implementation-format' of https://github.com/kdorsel/Tc…

5185aa2

…Black into implementation-format

Rework and add case support, if/else, finding multiline function calls

ed322b1

Multi line function calls inside an if don't work

Multiline function calls

2f7e612

Add initial support for multiline comments

b6e4c95

Roald87 reviewed Aug 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format implementation assignments, ifs #41

Format implementation assignments, ifs #41

kdorsel commented Jul 16, 2020

Roald87 left a comment

Roald87 Jul 26, 2020

kdorsel Jul 27, 2020

Roald87 Aug 11, 2020

Roald87 Jul 26, 2020

Roald87 commented Jul 26, 2020

kdorsel commented Jul 27, 2020

kdorsel commented Jul 30, 2020

Roald87 left a comment

Roald87 Aug 11, 2020

Roald87 Aug 11, 2020

Roald87 Aug 11, 2020

Roald87 Aug 11, 2020

Roald87 Aug 11, 2020

Roald87 Aug 11, 2020

Roald87 commented Aug 14, 2020

Format implementation assignments, ifs #41

Are you sure you want to change the base?

Format implementation assignments, ifs #41

Conversation

kdorsel commented Jul 16, 2020

Roald87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roald87 commented Jul 26, 2020

kdorsel commented Jul 27, 2020

kdorsel commented Jul 30, 2020

Roald87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roald87 commented Aug 14, 2020