Skip to content
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

Fix class keyword coloring #99

Merged
merged 1 commit into from
May 25, 2018
Merged

Fix class keyword coloring #99

merged 1 commit into from
May 25, 2018

Conversation

kborowinski
Copy link
Contributor

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

Fixes class keyword coloring when used as parameter
@kborowinski
Copy link
Contributor Author

Before:

image

After:

image

@omniomi omniomi merged commit 40d2d8f into PowerShell:master May 25, 2018
@kborowinski kborowinski deleted the patch-2 branch May 25, 2018 12:37
@vors
Copy link
Collaborator

vors commented May 25, 2018

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

@omniomi
Copy link
Collaborator

omniomi commented May 25, 2018

@vors they are included. if you look at an open one like: #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
Copy link
Collaborator

vors commented May 25, 2018

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
Copy link
Collaborator

omniomi commented May 25, 2018

@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
Copy link
Collaborator

vors commented May 25, 2018

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
Copy link
Collaborator

omniomi commented May 25, 2018

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.

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.

None yet

3 participants