Skip to content

Syntax bug fixes (#68, #77, #84, Range Operator) #100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

Syntax bug fixes (#68, #77, #84, Range Operator) #100

wants to merge 9 commits into from

Conversation

bcrotty
Copy link
Contributor

@bcrotty bcrotty commented Feb 26, 2015

No description provided.

@@ -234,7 +234,7 @@
</dict>
<dict>
<key>match</key>
<string>(?&lt;!\w)-([ci]?[lg][te]|eq|ne)</string>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This incorrectly allows -lexx

@bcrotty
Copy link
Contributor Author

bcrotty commented Feb 26, 2015

Why is the build failing? On the commits above, it looks like it's failing on the last commit (2f400dd), but it did the same thing the last pull request I tried (#91), and this is the same thing minus that pull's last commit. The changes are fairly simple. Did I miss something, or is there something wrong with the test?

@bcrotty
Copy link
Contributor Author

bcrotty commented Feb 26, 2015

It looks like this is failing because of the issue from #96. The build fails on support.function.powershell. Should I resubmit this PR with the reverted function syntax?

@vors
Copy link
Member

vors commented Feb 26, 2015

I merged #96. It may be related. We already have a bunch of similar problem https://github.com/SublimeText/PowerShell/blob/master/tests/pester/SyntaxHelper.psm1#L139
Ideally, case should not affect kind of scope. Last release, I didn't have time to fix all problems and just workaround them.

@vors
Copy link
Member

vors commented Feb 26, 2015

Please, include samples of syntax that used to be incorrect and now correct to tests\samples\test-file.ps1. It's both human and machine readable and including samples will reduce regressions in future.

@@ -522,7 +528,7 @@
<key>function</key>
<dict>
<key>begin</key>
<string>((?i:function|configuration|workflow))\s+((?:\p{L}|\d|_|-|\.)+)</string>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrectly treats a parameter -Function as a function definition

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool

function echo([string]$text) {
write-host $text
}

$file = join-path $env:SystemDrive "$([System.io.path]::GetRandomFileName()).ps1"
$ScriptBlock | Out-File $file -Force

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, revert. These are changes I added recently, did you have problems with merge from dev?

@vors
Copy link
Member

vors commented Feb 27, 2015

@bcrotty I probably should write-up how to run tests locally.

@guillermooo
Copy link
Member

@bcrotty I probably should write-up how to run tests locally.

@vors Yes, please! :-) I took a (very) quick look at your testing code, but couldn't figure out how to run the new syntax tests locally.

@guillermooo
Copy link
Member

(Or whether some previous step is needed at all; I thought so.)

@vors
Copy link
Member

vors commented Feb 27, 2015

#102

@bcrotty
Copy link
Contributor Author

bcrotty commented Feb 27, 2015

@vors Sorry about the test-file. Should be sorted out now.

@vors
Copy link
Member

vors commented Mar 2, 2015

no problem. I added instruction about local testing https://github.com/SublimeText/PowerShell/blob/dev/CONTRIBUTING.md#run-tests-locally

@vors
Copy link
Member

vors commented Mar 4, 2015

For now, you can unblock yourself with adding failed scopes to https://github.com/SublimeText/PowerShell/blob/dev/tests/pester/SyntaxHelper.psm1#L139

@vors
Copy link
Member

vors commented Mar 4, 2015

Or create a separate if above. This code require some time to dive in... Let me know if you are stuck with it.

@bcrotty
Copy link
Contributor Author

bcrotty commented Mar 4, 2015

@vors I might need some help. I added exclusions for support.function.powershell and interpolated.simple.source.powershell, but it doesn't work still. When it's doing its tests, is it using my branch or SublimeText:dev?

@vors
Copy link
Member

vors commented Mar 4, 2015

Sorry, I mislead you a little bit.
The current usage of $bugsExclude is to provide a common substrings (in form of regex) to consider different tokens the same, i.e. constant.numeric.scientific.powershell and support.constant.powershell. In your case, there is no common substring which can be used. You can ever add a separate if for your case or try to understand why that happens (you are way better in understanding sublime plist rules then me). It have to be fixed eventually.

@bcrotty
Copy link
Contributor Author

bcrotty commented Mar 4, 2015

Ok, I'll work on that. Also, @vors I can't run the tests locally. I get the below error. From the source code, it looks like ${env:SUBLIME_TEXT_VERSION} doesn't correctly resolve to anything. Is that not a universal environment variable?

2015-03-04 17_29_00-administrator_ windows powershell

@vors
Copy link
Member

vors commented Mar 4, 2015

This code leaves in https://github.com/randy3k/UnitTesting/blob/master/sbin/appveyor.ps1
Please, workaround it with $env:SUBLIME_TEXT_VERSION = "3"
I will update instruction to run tests locally, once you will succeed :)

@bcrotty
Copy link
Contributor Author

bcrotty commented Mar 5, 2015

I finally got it working, but none of the ${env:____} worked for me. Also, start-filedownload and 7z.exe could not be found. I have 7zip installed, but I guess it's not in my PATH or something. I downloaded and extracted the build manually.

I finally was able to run the test using the command:
& "C:\st\Data\Packages\UnitTesting\sbin\run.ps1" "PowerShell"

It looks like it copied PowerShell from the current directory that my PowerShell console was pointed to, which happened to be correct, but if I had wanted it to run on a different copy of the PowerShell code, I'm not sure how I would have done that.

@vors
Copy link
Member

vors commented Mar 5, 2015

You are absolutely right.
I actually should not point to appveyor.ps1, because it's heavily depends on build agent environment:

  • start-filedownload is a cmdlet provided by AppVeyor on build VMs.
  • AppVeyor has 7z.exe in $env:PATH

I think the right way would be contribute to https://github.com/randy3k/UnitTesting/blob/master/sbin and provide two scripts: for local run (?) and for appvoyer run.

@vors
Copy link
Member

vors commented May 6, 2015

Oh we need to merge this. Can you resolve merge conflicts?

@bcrotty
Copy link
Contributor Author

bcrotty commented May 6, 2015

I think that's why this never got merged in the first place. I'm pretty sure the commits themselves are good, but we need to add an exclusion to the test, and I could use some help with that.

@vors
Copy link
Member

vors commented May 6, 2015

Current tests framework is super complicated, involves cloning UnitTests repo, installing sublime texts, run python in-proc tests, serialize sublime scopes. I believe it can be replaced with scopes generation out-of-proc (maybe github library for that). It would greatly simplify troubleshooting.

@guillermooo
Copy link
Member

Just a heads up that a new syntax def using the new format .sublime-syntax is under work:

#127

The .sublime-syntax also includes the ability to test syntax defs. We need to integrate that with the tests in the PS package, though.

I think fixing bugs in the .tmLanguage is ok (as it will help GH hilite PS files better too), but I'm not so sure we should spend too much time improving tests, etc. around the old format?

@vors vors closed this May 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants