SublimeText / PowerShell

Support for the MS PowerShell programming language.
MIT License
313 stars 78 forks source link

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

Closed bcrotty closed 6 years ago

bcrotty commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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.

vors commented 9 years ago

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

guillermooo commented 9 years ago

@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 commented 9 years ago

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

vors commented 9 years ago

102

bcrotty commented 9 years ago

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

vors commented 9 years ago

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

vors commented 9 years ago

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

vors commented 9 years ago

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

bcrotty commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

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 commented 9 years ago

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

bcrotty commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

https://github.com/SublimeText/PowerShell/pull/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?