search

PowerShell / EditorSyntax

PowerShell syntax highlighting for editors (VS Code, Atom, SublimeText, TextMate, etc.) and GitHub!
MIT License
133 stars 45 forks source link

Fix class keyword coloring #99

Closed kborowinski closed 6 years ago

kborowinski commented 6 years ago

Fixes class keyword coloring when used as parameter (#43)

kborowinski commented 6 years ago

Before:

image

After:

image

vors commented 6 years ago

Should we try to include tests in PRs to prevent regressions?

omniomi commented 6 years ago

@vors they are included. if you look at an open one like: https://github.com/PowerShell/EditorSyntax/pull/101 it's under 'All checks have passed.' The red 'x' on the initial commit was due to a failed test.

Did this PR introduce regression? I didn't notice any and the tests passed.

vors commented 6 years ago

There was a bug and PR fixed it (yay!), but doesn't include a test. So there is nothing hypothetically preventing it from regressing in the future.

omniomi commented 6 years ago

@vors there were tests on this PR but because it's already merge they don't show up unless you click the check beside the commit:

pr

https://ci.appveyor.com/project/PowerShell/editorsyntax/build/1.0.44-jxgivpko

The one thing that's not being done is the tests part of the build doesn't upload test results so if a test fails the build fails but it doesn't specifically add anything to the tests tab. That's something we can fix.

vors commented 6 years ago

Sorry, I think we still not on the same page:

Let me rephrase what you are saying (how I understand it): "CI run the tests on this PR and they are passed". Is it right?

What I'm saying is: "There are no tests for this particular issue and it's a good idea to start including tests together with 'code' changes". Does it make sense?

omniomi commented 6 years ago

Ohhh! Yes. I agree.

I am keeping a list of these little fixes the add to the tests but we could require the tests be added as part of the PR.