search

mvextensions / mvbasic

MultiValue Basic extension for Visual Studio Code
MIT License
30 stars 16 forks source link

[FEATURE] Label jumping out of code block #105

Open brentlblair opened 4 years ago

brentlblair commented 4 years ago

Describe the bug Lint shows an error on both lines with "GOTO 208". 208 is trying to go out of scope ex TEST(24, 20): Invalid GOTO or GOSUB, jumping into/out of a block This code compiles without issue and works in our production jBase environment. In this snippet, we want to jump back to the input if we receive bad input from the user. I took this snippet from an existing program and just copied this portion of code to make sure that it was not falsely complaining about other code in the program.

Code samples and/or reproduceable test cases FOR P=1 TO NUM.LINES TOT.PICKED=TEMP2<P,3> ;Picked = Shipped IF TOT.PICKED > "0" THEN ;Don't put back parts not yet picked PN=TEMP2<P,2> SCANNED.BIN=TEMP2<P,1> LOC=TEMP2<P,20> READ PRD FROM F.PRODUCT,PN:"*":LOC ELSE PRD="" MFG=PRD<4> PART.NUM=PRD<19> BIN1=PRD<60,1> BIN2=PRD<60,2> PRINT @(0,1):WIPE:"Reverse Pick" PRINT @(0,2):WIPE:"Loc ":LOC PRINT @(0,3):WIPE:"Part# ":MFG:PART.NUM PRINT @(0,4):WIPE:"QTY ":TOT.PICKED PRINT @(0,5):WIPE:"Bin1 ":BIN1 PRINT @(0,6):WIPE:"Bin2 ":BIN2 208 PRINT BOTTOM.POS:"Bin Location": INPUT SOURCE.DESTINATION: SOURCE.DESTINATION.MODE="D" IF SOURCE.DESTINATION.ERROR#"" THEN PRINT BOTTOM.POS:SOURCE.DESTINATION.ERROR RQM(2) GOTO 208 END THIS.SCAN.QTY=TOT.PICKED LOC.QTY.MODE="R" IF LOC.QTY.ERROR#"" THEN PRINT BOTTOM.POS:LOC.QTY.ERROR RQM(2) GOTO 208 END END NEXT P

kpowick commented 4 years ago

Thanks, @brentlblair . This issue is related to issues #99 and #104, which I hope to resolve for the v1.0.7 release.

dthiot commented 4 years ago

I do not see this error on the released version of the extension that I have which is 2.0.5. However, I don’t see an “*” on the comment that is on the line with IF TOT.PICKED > “0” THEN line either. Do you have other code from outside of the FOR/NEXT loop that references the 208 label? [A screenshot of a cell phone Description automatically generated]

Please don’t take offense to this but I have a particular aversion to the use of GOTO statements. I would likely refactor your code as: [A screenshot of a cell phone Description automatically generated]

From: Brent Blair notifications@github.com Reply-To: mvextensions/mvbasic reply@reply.github.com Date: Wednesday, June 3, 2020 at 2:05 PM To: mvextensions/mvbasic mvbasic@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [mvextensions/mvbasic] [BUG] Label jumping out of code block (#105)

Describe the bug Lint shows an error on both lines with "GOTO 208". 208 is trying to go out of scope ex TEST(24, 20): Invalid GOTO or GOSUB, jumping into/out of a block This code compiles without issue and works in our production jBase environment. In this snippet, we want to jump back to the input if we receive bad input from the user. I took this snippet from an existing program and just copied this portion of code to make sure that it was not falsely complaining about other code in the program.

Code samples and/or reproduceable test cases FOR P=1 TO NUM.LINES TOT.PICKED=TEMP2<P,3> ;*Picked = Shipped IF TOT.PICKED > "0" THEN ;Don't put back parts not yet picked PN=TEMP2<P,2> SCANNED.BIN=TEMP2<P,1> LOC=TEMP2<P,20> READ PRD FROM F.PRODUCT,PN:"":LOC ELSE PRD="" MFG=PRD<4> PART.NUM=PRD<19> BIN1=PRD<60,1> BIN2=PRD<60,2> PRINT @(0,1):WIPE:"Reverse Pick" PRINT @(0,2):WIPE:"Loc ":LOC PRINT @(0,3):WIPE:"Part# ":MFG:PART.NUM PRINT @(0,4):WIPE:"QTY ":TOT.PICKED PRINT @(0,5):WIPE:"Bin1 ":BIN1 PRINT @(0,6):WIPE:"Bin2 ":BIN2 208 PRINT BOTTOM.POS:"Bin Location": INPUT SOURCE.DESTINATION: SOURCE.DESTINATION.MODE="D" IF SOURCE.DESTINATION.ERROR#"" THEN PRINT BOTTOM.POS:SOURCE.DESTINATION.ERROR RQM(2) GOTO 208 END THIS.SCAN.QTY=TOT.PICKED LOC.QTY.MODE="R" IF LOC.QTY.ERROR#"" THEN PRINT BOTTOM.POS:LOC.QTY.ERROR RQM(2) GOTO 208 END END NEXT P

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/mvextensions/mvbasic/issues/105, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACOODLYS2V2LJE6SEH3NPZ3RU2NIDANCNFSM4NR6DAKQ.

dthiot commented 4 years ago

I was surprised to see that my system had version 2.0.5 of the extension since I thought that 2.0.6 was the currently released version.

From: Kevin Powick notifications@github.com Reply-To: mvextensions/mvbasic reply@reply.github.com Date: Wednesday, June 3, 2020 at 2:48 PM To: mvextensions/mvbasic mvbasic@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [mvextensions/mvbasic] [BUG] Label jumping out of code block (#105)

Thanks, @brentlblairhttps://github.com/brentlblair . This issue is related to issues #99https://github.com/mvextensions/mvbasic/issues/99 and #104https://github.com/mvextensions/mvbasic/issues/104, which I hope to resolve for the v1.0.7 release.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/mvextensions/mvbasic/issues/105#issuecomment-638424072, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACOODL7I3WM2VF6KQKXL4JLRU2SHZANCNFSM4NR6DAKQ.

brentlblair commented 4 years ago

No, the only references to 208 are in the code block that I sent you.

Brent Blair http://www.linkedin.com/in/BrentLBlair

Vice President of IT Encompass Phone 954.474.0300 Fort Lauderdale, FL https://www.google.com/maps/place/Encompass+Parts+Distribution/@26.145357,-80.2482168,12.46z/data=!4m5!3m4!1s0x0:0xb2bfe23d3412449!8m2!3d26.0920996!4d-80.2409538

bblair@encompass.com encompass.com

On Wed, Jun 3, 2020 at 3:53 PM Dick Thiot notifications@github.com wrote:

I do not see this error on the released version of the extension that I have which is 2.0.5. However, I don’t see an “*” on the comment that is on the line with IF TOT.PICKED > “0” THEN line either. Do you have other code from outside of the FOR/NEXT loop that references the 208 label? [A screenshot of a cell phone Description automatically generated]

Please don’t take offense to this but I have a particular aversion to the use of GOTO statements. I would likely refactor your code as: [A screenshot of a cell phone Description automatically generated]

From: Brent Blair notifications@github.com Reply-To: mvextensions/mvbasic reply@reply.github.com Date: Wednesday, June 3, 2020 at 2:05 PM To: mvextensions/mvbasic mvbasic@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [mvextensions/mvbasic] [BUG] Label jumping out of code block (#105)

Describe the bug Lint shows an error on both lines with "GOTO 208". 208 is trying to go out of scope ex TEST(24, 20): Invalid GOTO or GOSUB, jumping into/out of a block This code compiles without issue and works in our production jBase environment. In this snippet, we want to jump back to the input if we receive bad input from the user. I took this snippet from an existing program and just copied this portion of code to make sure that it was not falsely complaining about other code in the program.

Code samples and/or reproduceable test cases FOR P=1 TO NUM.LINES TOT.PICKED=TEMP2<P,3> ;*Picked = Shipped IF TOT.PICKED > "0" THEN ;Don't put back parts not yet picked PN=TEMP2<P,2> SCANNED.BIN=TEMP2<P,1> LOC=TEMP2<P,20> READ PRD FROM F.PRODUCT,PN:"":LOC ELSE PRD="" MFG=PRD<4> PART.NUM=PRD<19> BIN1=PRD<60,1> BIN2=PRD<60,2> PRINT @(0,1):WIPE:"Reverse Pick" PRINT @(0,2):WIPE:"Loc ":LOC PRINT @(0,3):WIPE:"Part# ":MFG:PART.NUM PRINT @(0,4):WIPE:"QTY ":TOT.PICKED PRINT @(0,5):WIPE:"Bin1 ":BIN1 PRINT @(0,6):WIPE:"Bin2 ":BIN2 208 PRINT BOTTOM.POS:"Bin Location": INPUT SOURCE.DESTINATION: SOURCE.DESTINATION.MODE="D" IF SOURCE.DESTINATION.ERROR#"" THEN PRINT BOTTOM.POS:SOURCE.DESTINATION.ERROR RQM(2) GOTO 208 END THIS.SCAN.QTY=TOT.PICKED LOC.QTY.MODE="R" IF LOC.QTY.ERROR#"" THEN PRINT BOTTOM.POS:LOC.QTY.ERROR RQM(2) GOTO 208 END END NEXT P

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub< https://github.com/mvextensions/mvbasic/issues/105>, or unsubscribe< https://github.com/notifications/unsubscribe-auth/ACOODLYS2V2LJE6SEH3NPZ3RU2NIDANCNFSM4NR6DAKQ>.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvextensions/mvbasic/issues/105#issuecomment-638427008, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFRDR4MVNPCYDR3XS3AMABDRU2S5DANCNFSM4NR6DAKQ .

kpowick commented 4 years ago

I was surprised to see that my system had version 2.0.5 of the extension since I thought that 2.0.6 was the currently released version.

Yes, 2.0.6 is the released version. I'm surprised you have 2.0.5 ;)

dthiot commented 4 years ago

I uninstalled and reinstalled and I now have 2.0.6. However, I don’t see the error that Brent is seeing. The formatter does not properly indent the code either. [A screen shot of a computer Description automatically generated]

From: Kevin Powick notifications@github.com Reply-To: mvextensions/mvbasic reply@reply.github.com Date: Wednesday, June 3, 2020 at 2:59 PM To: mvextensions/mvbasic mvbasic@noreply.github.com Cc: Dick Thiot dthiot@mavsys.com, Comment comment@noreply.github.com Subject: Re: [mvextensions/mvbasic] [BUG] Label jumping out of code block (#105)

I was surprised to see that my system had version 2.0.5 of the extension since I thought that 2.0.6 was the currently released version.

Yes, 2.0.6 is the released version. I'm surprised you have 2.0.5 ;)

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/mvextensions/mvbasic/issues/105#issuecomment-638429679, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACOODL25NQFYUL2YJ3S44SLRU2TQ3ANCNFSM4NR6DAKQ.

kpowick commented 4 years ago

I uninstalled and reinstalled and I now have 2.0.6. However, I don’t see the error that Brent is seeing. The formatter does not properly indent the code either.

I have 2.0.6 installed. Formatting is ok, and the error is as Brent describes. Not sure what's up with your local system. Try VSC restart?

[A screen shot of a computer Description automatically generated]

Your screen shots are not being included.

ianmcgowan commented 4 years ago

Thanks, @brentlblair . This issue is related to issues #99 and #104, which I hope to resolve for the v1.0.7 release.

Kevin, not sure this is related to those issues, there's explicit code to check for jumping into or out of a block (technically any code at a different indent level) in the linter. I sort of agree with the intent of that, but it's quite opinionated of the extension to flag it as an error - it's legal syntax and there's plenty of legacy code around that does worse :_) The intent of this check is good, but it's perhaps a mistake to flag legal code as an error (maybe a warning makes more sense?).

                if (
                  labelMatch.Level != RowLevel[i] &&
                  labelMatch.Level > 1 &&
                  ignoreGotoScope === false
                ) {
                  // jumping into or out of a loop

Here's a more minimal test case that is legal syntax, no label issues, but flags the goto as an error:

FOR X=1 TO 20
  CRT X
  IF X > 10 THEN
LOOPING:
    CRT 'ENTER / TO QUIT: X=':X
    INPUT AAA
    IF AAA#'/' THEN
      GOTO LOOPING
    END
  END
NEXT X

Because the checking is happening based purely on tracking the "level" of each line, you can fool this check by adding a dummy for loop to push the label to the same level as the goto and the error goes away.

FOR X=1 TO 20
  IF X > 10 THEN
    FOR F=1 TO 1
LOOPING:
    NEXT F
    PRINT 'ENTER / TO QUIT: X=':X
    INPUT AAA
    IF AAA#'/' THEN
      GOTO LOOPING
    END
  END
NEXT X
brentlblair commented 4 years ago

My vote would be to add extension settings to control this. I use PHPStorm, and it has a whole section of user settings around linter. I don’t want to see this flagged as an error or warning, whereas other users might. I have to imagine there will be other similar situations with the linter where we could all debate what should be considered a warning, error, or nothing.

On Fri, Jun 5, 2020 at 12:18 AM Ian McGowan notifications@github.com wrote:

Thanks, @brentlblair https://github.com/brentlblair . This issue is related to issues #99 https://github.com/mvextensions/mvbasic/issues/99 and #104 https://github.com/mvextensions/mvbasic/issues/104, which I hope to resolve for the v1.0.7 release.

Kevin, not sure this is related to those issues, there's explicit code to check for jumping into or out of a block (technically any code at a different indent level) in the linter. I sort of agree with the intent of that, but it's quite opinionated of the extension to flag it as an error - it's legal syntax and there's plenty of legacy code around that does worse :_) The intent of this check is good, but it's perhaps a mistake to flag legal code as an error (maybe a warning makes more sense?).

            if (
              labelMatch.Level != RowLevel[i] &&
              labelMatch.Level > 1 &&
              ignoreGotoScope === false
            ) {
              // jumping into or out of a loop

Here's a more minimal test case that is legal syntax, no label issues, but flags the goto as an error:

FOR X=1 TO 20 CRT X IF X > 10 THEN LOOPING: CRT 'ENTER / TO QUIT: X=':X INPUT AAA IF AAA#'/' THEN GOTO LOOPING END END NEXT X

Because the checking is happening based purely on tracking the "level" of each line, you can fool this check by adding a dummy for loop to push the label to the same level as the goto and the error goes away.

FOR X=1 TO 20 IF X > 10 THEN FOR F=1 TO 1 LOOPING: NEXT F PRINT 'ENTER / TO QUIT: X=':X INPUT AAA IF AAA#'/' THEN GOTO LOOPING END END NEXT X

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvextensions/mvbasic/issues/105#issuecomment-639248680, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFRDR4PH5XWTZS7M3CC7MITRVBW27ANCNFSM4NR6DAKQ .

--

Brent Blair http://www.linkedin.com/in/BrentLBlair

Vice President of IT Encompass Phone 954.474.0300 Fort Lauderdale, FL https://www.google.com/maps/place/Encompass+Parts+Distribution/@26.145357,-80.2482168,12.46z/data=!4m5!3m4!1s0x0:0xb2bfe23d3412449!8m2!3d26.0920996!4d-80.2409538

bblair@encompass.com encompass.com

kpowick commented 4 years ago

Kevin, not sure this is related to those issues, there's explicit code to check for jumping into or out of a block (technically any code at a different indent level) in the linter. I sort of agree with the intent of that, but it's quite opinionated of the extension to flag it as an error

I completely agree and was actually thinking of removing it entirely. It might be "bad form", but if the compiler allows it, the extension shouldn't flag it as an error.

I think that future versions of the extension could include options to warn about such coding practices, but not at this stage of its development.

ianmcgowan commented 4 years ago

There is a flag, ignoreGotoScope, you can set it by adding this to your settings (workspace or global). Maybe we should set the default for this to true?

"MVBasic.ignoreGotoScope": true,

On Thu, Jun 4, 2020 at 10:04 PM Brent Blair notifications@github.com wrote:

My vote would be to add extension settings to control this. I use PHPStorm, and it has a whole section of user settings around linter. I don’t want to see this flagged as an error or warning, whereas other users might. I have to imagine there will be other similar situations with the linter where we could all debate what should be considered a warning, error, or nothing.

On Fri, Jun 5, 2020 at 12:18 AM Ian McGowan notifications@github.com wrote:

Thanks, @brentlblair https://github.com/brentlblair . This issue is related to issues #99 <https://github.com/mvextensions/mvbasic/issues/99

and #104 https://github.com/mvextensions/mvbasic/issues/104, which I hope to resolve for the v1.0.7 release.

Kevin, not sure this is related to those issues, there's explicit code to check for jumping into or out of a block (technically any code at a different indent level) in the linter. I sort of agree with the intent of that, but it's quite opinionated of the extension to flag it as an error

it's legal syntax and there's plenty of legacy code around that does worse :_) The intent of this check is good, but it's perhaps a mistake to flag legal code as an error (maybe a warning makes more sense?).

if ( labelMatch.Level != RowLevel[i] && labelMatch.Level > 1 && ignoreGotoScope === false ) { // jumping into or out of a loop

Here's a more minimal test case that is legal syntax, no label issues, but flags the goto as an error:

FOR X=1 TO 20 CRT X IF X > 10 THEN LOOPING: CRT 'ENTER / TO QUIT: X=':X INPUT AAA IF AAA#'/' THEN GOTO LOOPING END END NEXT X

Because the checking is happening based purely on tracking the "level" of each line, you can fool this check by adding a dummy for loop to push the label to the same level as the goto and the error goes away.

FOR X=1 TO 20 IF X > 10 THEN FOR F=1 TO 1 LOOPING: NEXT F PRINT 'ENTER / TO QUIT: X=':X INPUT AAA IF AAA#'/' THEN GOTO LOOPING END END NEXT X

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/mvextensions/mvbasic/issues/105#issuecomment-639248680 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AFRDR4PH5XWTZS7M3CC7MITRVBW27ANCNFSM4NR6DAKQ

.

--

Brent Blair http://www.linkedin.com/in/BrentLBlair

Vice President of IT Encompass Phone 954.474.0300 Fort Lauderdale, FL < https://www.google.com/maps/place/Encompass+Parts+Distribution/@26.145357,-80.2482168,12.46z/data=!4m5!3m4!1s0x0:0xb2bfe23d3412449!8m2!3d26.0920996!4d-80.2409538

bblair@encompass.com encompass.com

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mvextensions/mvbasic/issues/105#issuecomment-639260486, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADD3DZZ7L4KACYBISMFVELRVB4FXANCNFSM4NR6DAKQ .

brentlblair commented 4 years ago

Sorry, I didn't notice that in the settings, you can close this issue. I do agree that it should be changed either way from an error to a warning since it compiles. I changed it to true, so it would be more of a suggestion for anyone who leaves the setting false.

Brent Blair http://www.linkedin.com/in/BrentLBlair

Vice President of IT Encompass Phone 954.474.0300 Fort Lauderdale, FL https://www.google.com/maps/place/Encompass+Parts+Distribution/@26.145357,-80.2482168,12.46z/data=!4m5!3m4!1s0x0:0xb2bfe23d3412449!8m2!3d26.0920996!4d-80.2409538

bblair@encompass.com encompass.com

On Fri, Jun 5, 2020 at 10:18 AM Ian McGowan notifications@github.com wrote:

There is a flag, ignoreGotoScope, you can set it by adding this to your settings (workspace or global):

"MVBasic.ignoreGotoScope": false,

On Thu, Jun 4, 2020 at 10:04 PM Brent Blair notifications@github.com wrote:

My vote would be to add extension settings to control this. I use PHPStorm, and it has a whole section of user settings around linter. I don’t want to see this flagged as an error or warning, whereas other users might. I have to imagine there will be other similar situations with the linter where we could all debate what should be considered a warning, error, or nothing.

On Fri, Jun 5, 2020 at 12:18 AM Ian McGowan notifications@github.com wrote:

Thanks, @brentlblair https://github.com/brentlblair . This issue is related to issues #99 < https://github.com/mvextensions/mvbasic/issues/99

and #104 https://github.com/mvextensions/mvbasic/issues/104, which I hope to resolve for the v1.0.7 release.

Kevin, not sure this is related to those issues, there's explicit code to check for jumping into or out of a block (technically any code at a different indent level) in the linter. I sort of agree with the intent of that, but it's quite opinionated of the extension to flag it as an error

it's legal syntax and there's plenty of legacy code around that does worse :_) The intent of this check is good, but it's perhaps a mistake to flag legal code as an error (maybe a warning makes more sense?).

if ( labelMatch.Level != RowLevel[i] && labelMatch.Level > 1 && ignoreGotoScope === false ) { // jumping into or out of a loop

Here's a more minimal test case that is legal syntax, no label issues, but flags the goto as an error:

FOR X=1 TO 20 CRT X IF X > 10 THEN LOOPING: CRT 'ENTER / TO QUIT: X=':X INPUT AAA IF AAA#'/' THEN GOTO LOOPING END END NEXT X

Because the checking is happening based purely on tracking the "level" of each line, you can fool this check by adding a dummy for loop to push the label to the same level as the goto and the error goes away.

FOR X=1 TO 20 IF X > 10 THEN FOR F=1 TO 1 LOOPING: NEXT F PRINT 'ENTER / TO QUIT: X=':X INPUT AAA IF AAA#'/' THEN GOTO LOOPING END END NEXT X

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/mvextensions/mvbasic/issues/105#issuecomment-639248680

, or unsubscribe <

https://github.com/notifications/unsubscribe-auth/AFRDR4PH5XWTZS7M3CC7MITRVBW27ANCNFSM4NR6DAKQ

.

--

Brent Blair http://www.linkedin.com/in/BrentLBlair

Vice President of IT Encompass Phone 954.474.0300 Fort Lauderdale, FL <

https://www.google.com/maps/place/Encompass+Parts+Distribution/@26.145357,-80.2482168,12.46z/data=!4m5!3m4!1s0x0:0xb2bfe23d3412449!8m2!3d26.0920996!4d-80.2409538

bblair@encompass.com encompass.com

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/mvextensions/mvbasic/issues/105#issuecomment-639260486 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AADD3DZZ7L4KACYBISMFVELRVB4FXANCNFSM4NR6DAKQ

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvextensions/mvbasic/issues/105#issuecomment-639525212, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFRDR4JRIY2X5YJHUNUYPFDRVD5DNANCNFSM4NR6DAKQ .

pschellenbach commented 4 years ago

This setting may need to have 3 choices: ignore, warning or error. The MVON# extension is the ancestor of this extension, and in the MVON# version of BASIC, jumping out of a code block is indeed an error. For other MV flavors it could be a warning or just ignored depending on policy.

ianmcgowan commented 4 years ago

I completely agree and was actually thinking of removing it entirely. It might be "bad form", but if the compiler allows it, the extension shouldn't flag it as an error. I think that future versions of the extension could include options to warn about such coding practices, but not at this stage of its development.

Especially since the linting is completely "lexical" and doesn't really consider the structure of the program at all. It would be awesome to actually parse the program into tokens and make the linting more aware of the structure, but that's a non-trivial task. Pete S. has a parser written in dotnet that might make a starting point after converting to typescript.

ianmcgowan commented 4 years ago

This setting may need to have 3 choices: ignore, warning or error. The MVON# extension is the ancestor of this extension, and in the MVON# version of BASIC, jumping out of a code block is indeed an error. For other MV flavors it could be a warning or just ignored depending on policy.

That's good to know! I retract my "opinionated" comment :-) It does make clear how challenging parsing the different flavors will be. I'd vote for vanilla-R83 linting to start!

dthiot commented 4 years ago

I may be wrong in this but my understanding of MVON# is that you can GOTO outside of a structure (IF/END, FOR/NEXT, LOOP/REPEAT, etc) but you cannot GOTO the middle of a structure. It is not something that C# allows and MVON# is transpiled to C# and then compiled with the .NET compiler.

From: Ian McGowan notifications@github.com Reply-To: mvextensions/mvbasic reply@reply.github.com Date: Friday, June 5, 2020 at 10:41 AM To: mvextensions/mvbasic mvbasic@noreply.github.com Cc: Dick Thiot dthiot@mavsys.com, Comment comment@noreply.github.com Subject: Re: [mvextensions/mvbasic] [BUG] Label jumping out of code block (#105)

This setting may need to have 3 choices: ignore, warning or error. The MVON# extension is the ancestor of this extension, and in the MVON# version of BASIC, jumping out of a code block is indeed an error. For other MV flavors it could be a warning or just ignored depending on policy.

That's good to know! I retract my "opinionated" comment :-) It does make clear how challenging parsing the different flavors will be. I'd vote for vanilla-R83 linting to start!

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/mvextensions/mvbasic/issues/105#issuecomment-639575162, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACOODLY4CHJH3WIVG73MDETRVEG3RANCNFSM4NR6DAKQ.

ianmcgowan commented 4 years ago

My vote would be to add extension settings to control this. I use PHPStorm, and it has a whole section of user settings around linter. I don’t want to see this flagged as an error or warning, whereas other users might. I have to imagine there will be other similar situations with the linter where we could all debate what should be considered a warning, error, or nothing.

That's a really good idea. I can imagine a lot more settings like ignoreGotoScope. And it would be nice to have a hierarchy if possible: MVBasic.linting.GotoScope = "warning", MVBasic.linting.MissingLabels = "ignore" etc. Might skip some goto debates that way, and also provide an option for folks with ancient codebases to turn off the nagging.

kpowick commented 4 years ago

There seems to be several things to consider here. Since we have the ignoreGoToScope option, perhaps in the interest of getting v2.0.7 out soon, we should revisit this item for v2.0.8. Though maybe for now, the linter can flag this as an warning instead of an error.

ianmcgowan commented 4 years ago

There seems to be several things to consider here. Since we have the ignoreGoToScope option, perhaps in the interest of getting v.1.0.7 out soon, we should revisit this item for v 1.0.8. Though maybe for now, the linter can flag this as an warning instead of an error.

Agreed. Brett is ok with using the option for now, and mass changes to the linter is something that will take a while to work through and test (if it's even considered a good idea). Vote to close?

kpowick commented 4 years ago

Vote to close?

I'll just remove the 2.0.7 milestone and change it from a bug to an FEATURE request.

kpowick commented 4 years ago

For 2.0.7, this scope jumping message from the linter is changed from an Error to a Warning. As noted, this check can be enabled/disabled via the extension settings option ignoreGoToScope.

brentlblair commented 4 years ago

Not sure if you are already aware, but my vscode updated to 2.0.7 today, and now ALL of my FOR NEXT loops show the error of missing NEXT statement. Just to double check, I created a new program with just the simple loop below, and it still shows the error. FOR X=1 TO 2 PRINT X NEXT X [image: Code_g938A9s9Rq.png]

Brent Blair http://www.linkedin.com/in/BrentLBlair

Vice President of IT Encompass Phone 954.474.0300 Fort Lauderdale, FL https://www.google.com/maps/place/Encompass+Parts+Distribution/@26.145357,-80.2482168,12.46z/data=!4m5!3m4!1s0x0:0xb2bfe23d3412449!8m2!3d26.0920996!4d-80.2409538

bblair@encompass.com encompass.com

On Wed, Jun 10, 2020 at 11:21 AM Kevin Powick notifications@github.com wrote:

For 2.0.7, this scope jumping message from the linter is changed from an Error to a Warning. As noted, this check can be enabled/disabled via the extension settings option ignoreGoToScope.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvextensions/mvbasic/issues/105#issuecomment-642079882, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFRDR4J6GJRTHWMCNET2MDTRV6QGZANCNFSM4NR6DAKQ .

dthiot commented 4 years ago

Brent,

Yes, 2.0.7 was released today. My regular VSCode (not dev) updated but required a reload. I took your code and pasted it into a new document and it does not show any error. I don’t think that it is directly related to the ignoreGoToScope setting because it doesn’t matter if I have this enabled or disabled. I’m not sure what other options you have different from what I have. I am going to check this on a generic system a little later today that hasn’t really had any VS Code configuration customization done and let you know unless Kevin, Mike or someone else replies sooner.

Dick

From: Brent Blair notifications@github.com Reply-To: mvextensions/mvbasic reply@reply.github.com Date: Thursday, June 11, 2020 at 11:41 AM To: mvextensions/mvbasic mvbasic@noreply.github.com Cc: Dick Thiot dthiot@mavsys.com, Comment comment@noreply.github.com Subject: Re: [mvextensions/mvbasic] [FEATURE] [was bug] Label jumping out of code block (#105)

Not sure if you are already aware, but my vscode updated to 2.0.7 today, and now ALL of my FOR NEXT loops show the error of missing NEXT statement. Just to double check, I created a new program with just the simple loop below, and it still shows the error. FOR X=1 TO 2 PRINT X NEXT X [image: Code_g938A9s9Rq.png]

Brent Blair http://www.linkedin.com/in/BrentLBlair

Vice President of IT Encompass Phone 954.474.0300 Fort Lauderdale, FL https://www.google.com/maps/place/Encompass+Parts+Distribution/@26.145357,-80.2482168,12.46z/data=!4m5!3m4!1s0x0:0xb2bfe23d3412449!8m2!3d26.0920996!4d-80.2409538

bblair@encompass.com encompass.com

On Wed, Jun 10, 2020 at 11:21 AM Kevin Powick notifications@github.com wrote:

For 2.0.7, this scope jumping message from the linter is changed from an Error to a Warning. As noted, this check can be enabled/disabled via the extension settings option ignoreGoToScope.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvextensions/mvbasic/issues/105#issuecomment-642079882, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFRDR4J6GJRTHWMCNET2MDTRV6QGZANCNFSM4NR6DAKQ .

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/mvextensions/mvbasic/issues/105#issuecomment-642798578, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACOODL26Y3UQORSGKLE6LFDRWECKVANCNFSM4NR6DAKQ.

brentlblair commented 4 years ago

We use Accuterm to drive VS Code and setup the workspace on the fly. This is the json file that drives the setup. Hope this helps.

// AccuTerm VSCode Workspace Template // // To change the maximum number of items to list from any file, // change RestFS.MaxItems. To show items in a dictionary level // file, change RestFS.SelAttr to 208. { "folders":[{"uri":"RestFS:/","name":""}], "extensions":{"recommendations":["mvextensions.mvbasic"]}, "settings":{ "files.associations":{"*":"mvbasic"}, "files.exclude":{ "/.git": true, "/.vscode": true, "/node_modules": true, "/pom.xml": true}, "files.eol":"\n", "MVBasic.ServerName":"", "MVBasic.Account":"", "MVBasic.RestPath":"http://localhost:3181/mvsvr/restfs", "MVBasic.indent":3, "MVBasic.margin":5, "MVBasic.useCamelCase":false, "MVBasic.ignoreGotoScope":true, "MVBasic.UseRestFS":true, "MVBasic.languageType":"", "MVBasic.RestFS.AutoConnect":true, "MVBasic.RestFS.RestAPI":1, "MVBasic.RestFS.MaxItems":5000, "MVBasic.RestFS.SelAttr":32976, "MVBasic.RestFS.CaseSensitive":, "MVBasic.EditFiles":[

] } } } *Brent Blair* Vice President of IT Encompass Phone 954.474.0300 Fort Lauderdale, FL bblair@encompass.com encompass.com On Thu, Jun 11, 2020 at 12:48 PM Dick Thiot wrote: > Brent, > > Yes, 2.0.7 was released today. My regular VSCode (not dev) updated but > required a reload. I took your code and pasted it into a new document and > it does not show any error. I don’t think that it is directly related to > the ignoreGoToScope setting because it doesn’t matter if I have this > enabled or disabled. I’m not sure what other options you have different > from what I have. I am going to check this on a generic system a little > later today that hasn’t really had any VS Code configuration customization > done and let you know unless Kevin, Mike or someone else replies sooner. > > Dick > > From: Brent Blair > Reply-To: mvextensions/mvbasic > Date: Thursday, June 11, 2020 at 11:41 AM > To: mvextensions/mvbasic > Cc: Dick Thiot , Comment > Subject: Re: [mvextensions/mvbasic] [FEATURE] [was bug] Label jumping out > of code block (#105) > > Not sure if you are already aware, but my vscode updated to 2.0.7 today, > and now ALL of my FOR NEXT loops show the error of missing NEXT statement. > Just to double check, I created a new program with just the simple loop > below, and it still shows the error. > FOR X=1 TO 2 > PRINT X > NEXT X > [image: Code_g938A9s9Rq.png] > > *Brent Blair* > > Vice President of IT > Encompass > Phone 954.474.0300 > Fort Lauderdale, FL > < > https://www.google.com/maps/place/Encompass+Parts+Distribution/@26.145357,-80.2482168,12.46z/data=!4m5!3m4!1s0x0:0xb2bfe23d3412449!8m2!3d26.0920996!4d-80.2409538> > > > bblair@encompass.com > encompass.com > > > On Wed, Jun 10, 2020 at 11:21 AM Kevin Powick > wrote: > > > For 2.0.7, this scope jumping message from the linter is changed from an > > Error to a Warning. As noted, this check can be enabled/disabled via the > > extension settings option ignoreGoToScope. > > > > — > > You are receiving this because you were mentioned. > > Reply to this email directly, view it on GitHub > > < > https://github.com/mvextensions/mvbasic/issues/105#issuecomment-642079882>, > > > or unsubscribe > > < > https://github.com/notifications/unsubscribe-auth/AFRDR4J6GJRTHWMCNET2MDTRV6QGZANCNFSM4NR6DAKQ> > > > . > > > > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub< > https://github.com/mvextensions/mvbasic/issues/105#issuecomment-642798578>, > or unsubscribe< > https://github.com/notifications/unsubscribe-auth/ACOODL26Y3UQORSGKLE6LFDRWECKVANCNFSM4NR6DAKQ>. > > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > , > or unsubscribe > > . >
kpowick commented 4 years ago

@brentlblair Wow. That is terrible, and I'm sorry.

I tested a bunch of sample programs and coding styles, but none where people had no whitespace between their equal signs -- FOR X=1 TO 10 vs FOR X = 1 TO 10.

Short of getting out a new release, or hotfix, this problem can be resolved by adding the whitespace around your equal signs. However, I understand this may be a coding style change you do not wish to make.

Again, I'm sorry about this. I'll get it fixed-up ASAP and we'll, hopefully, get @itsxallwater to build a hotfix that you can download.

itsxallwater commented 4 years ago

Yeah we can get a release in to address this quickly. Thanks for jumping on it guys.

brentlblair commented 4 years ago

I’m sure it’s not easy to guess all of the different styles for all of the different flavors of pick. We generally try to avoid extra white space. With all of the code that we have, I’m sure it’s both ways. Just let me know when you have a hot fix. As a department we want to use this exclusively once we get past the false positives. I appreciate what you guys are doing with this product.

On Thu, Jun 11, 2020 at 1:06 PM Kevin Powick notifications@github.com wrote:

@brentlblair https://github.com/brentlblair Wow. That is terrible, and I'm sorry.

I tested a bunch of sample programs and coding styles, but none where people had no whitespace between their equal signs -- FOR X=1 TO 10 vs FOR X = 1 TO 10.

Short of getting out a new release, or hotfix, this problem can be resolved by adding the whitespace around your equal signs. However, I understand this may be a coding style change you do not wish to make.

Again, I'm sorry about this. I'll get it fixed-up ASAP and we'll, hopefully, get @itsxallwater https://github.com/itsxallwater to build a hotfix that you can download.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mvextensions/mvbasic/issues/105#issuecomment-642814229, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFRDR4OHR3W4NYYTRTGGEOLRWEFI3ANCNFSM4NR6DAKQ .

--

Brent Blair http://www.linkedin.com/in/BrentLBlair

Vice President of IT Encompass Phone 954.474.0300 Fort Lauderdale, FL https://www.google.com/maps/place/Encompass+Parts+Distribution/@26.145357,-80.2482168,12.46z/data=!4m5!3m4!1s0x0:0xb2bfe23d3412449!8m2!3d26.0920996!4d-80.2409538

bblair@encompass.com encompass.com

kpowick commented 4 years ago

@brentlblair

I've opened a new issue #107 for this bug.

Again, sorry. Thanks for your understanding.

dthiot commented 4 years ago

Kevin,

Why don’t I see the error in Brent’s code snippet?

Dick

From: Kevin Powick notifications@github.com Reply-To: mvextensions/mvbasic reply@reply.github.com Date: Thursday, June 11, 2020 at 12:24 PM To: mvextensions/mvbasic mvbasic@noreply.github.com Cc: Dick Thiot dthiot@mavsys.com, Comment comment@noreply.github.com Subject: Re: [mvextensions/mvbasic] [FEATURE] [was bug] Label jumping out of code block (#105)

@brentlblairhttps://github.com/brentlblair

I've opened a new issue #107https://github.com/mvextensions/mvbasic/issues/107 for this bug.

Again, sorry. Thanks for your understanding.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/mvextensions/mvbasic/issues/105#issuecomment-642823414, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACOODL33ROLI3AEILXDE4KLRWEHMNANCNFSM4NR6DAKQ.

kpowick commented 4 years ago

@dthiot I'm unsure. I've got version 2.0.7 as auto-updated by VS Code and the error happens for me as Brent describes.

I have a screenshot you can review in issue #107. Let's continue this discussion under that issue number.

itsxallwater commented 4 years ago

Sorry if I'm being dense, this ticket is still open too are we leaving it as-is to address a separate issue?

kpowick commented 4 years ago

This was an enhancement request related to Scope Jumping. Unfortunately, we've gone a little OT in the comments, which is why I opened issue #107.

Maybe I should close this one and open a new "clean" enhancement request for Scope Jumping?

itsxallwater commented 4 years ago

I'm good with it. Thanks for staying on point @kpowick!

dthiot commented 4 years ago

I updated to 2.0.8 and still don’t have an error on the FOR/NEXT loop sample code from Brent, so the update didn’t break something that was working.

From: Mike Wright notifications@github.com Reply-To: mvextensions/mvbasic reply@reply.github.com Date: Thursday, June 11, 2020 at 12:42 PM To: mvextensions/mvbasic mvbasic@noreply.github.com Cc: Dick Thiot dthiot@mavsys.com, Mention mention@noreply.github.com Subject: Re: [mvextensions/mvbasic] [FEATURE] [was bug] Label jumping out of code block (#105)

I'm good with it. Thanks for staying on point @kpowickhttps://github.com/kpowick!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mvextensions/mvbasic/issues/105#issuecomment-642832178, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACOODL2AZEPGME4XN7AKWKLRWEJPZANCNFSM4NR6DAKQ.