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

Finalise new -ReportSummary switch #895

Merged
merged 14 commits into from
Mar 2, 2018

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Feb 17, 2018

PR Summary

Fixes #857 and closes #868

This is the tweak of the new ReportSummary of PR #868 by @StingyJack that:

  • fixes the CI failures by adding the markdown documentation of the new -ReportSummary switch and adapting the library test function mock for consistency
  • add tests for it
  • tweak report summary with colour and wording.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • User facing documentation needed: Added help to cmdlet description
  • Change is not breaking
  • Make sure you've added a new test if existing tests do not effectively test the code changed
  • This PR is ready to merge and is not work in progress. It is ready but we need to merge PR 892 with the fix for Pester v4 in the Visual Studio 2017 image first
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Copy link
Member

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

I think the summary is fine and has some utility. It does not need to be used, but some may find it very useful.
However, one of the things we need to keep in mind when color is used explicitly is that we keep accessibility in mind as the colors we choose may not be accessible for all.
Further, some window color themes may be set in such a way that the chosen colors may not be visible at all. For example, a red background would have difficulties with red foreground text.
I think that the colors used by Write-Host (and host.writeline) need to be those found in $Host.PrivateData rather than hardcoded. We may not be able to provide the ultimate in configuration, but we can use the scheme in use. This will provide the best accessibility for our customers. It is also consistent with the way PowerShell uses colors.

@bergmeister
Copy link
Collaborator Author

bergmeister commented Feb 21, 2018

Sounds reasonable. I was actually thinking about the colour problem but saw it at first rather as an edge case. I will take a look at your pointer with $Host.PrivateData, thanks. A minimum viable implementation would also be to drop the colouring and add that later since I don't know how close we are to the next release?
Once I'm done with the colouring, we should make sure that we don't forget to give @StingyJack credit for his initial work (e.g. by mentioning it in the release notes) since it is not his fault that the PSSA CI/test system is not very beginner friendly.

@bergmeister bergmeister changed the title Finalise new -ReportSummary switch [WIP] Finalise new -ReportSummary switch Feb 24, 2018
@bergmeister bergmeister changed the title [WIP] Finalise new -ReportSummary switch Finalise new -ReportSummary switch Feb 25, 2018
@bergmeister
Copy link
Collaborator Author

bergmeister commented Feb 25, 2018

@JamesWTruher I have applied your suggestions and the PR is up for re-rereview
@StingyJack Can you also please check that the colouring works well for you in VS?

@bergmeister bergmeister added this to the 1.17 milestone Feb 26, 2018
Copy link
Member

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

this looks fine, please go ahead and merge!

@bergmeister
Copy link
Collaborator Author

@JamesWTruher Thanks but GitHub says that I do not have write access to protected branches like development and master (only 'normal' branches of PSSA) and therefore cannot complete/merge the PR myself.

@bergmeister bergmeister self-assigned this Feb 28, 2018
@JamesWTruher
Copy link
Member

I'll try to fix the merge permissions - I want to be sure that you can do that.

@JamesWTruher JamesWTruher merged commit 367384a into PowerShell:development Mar 2, 2018
@bergmeister
Copy link
Collaborator Author

Thanks. But we should still continue the process of getting approval from each other in PRs before merging.

@StingyJack
Copy link

StingyJack commented Mar 6, 2018

I'm not exactly sure how to get whatever changes you made to the coloring to see if they work ok. I added your branch as a remote and tried to fetch / pull and it says its doing stuff but the history still says my commit was the latest thing.

Nevermind, I think I found it and will check when I get into the office.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there a way to have Invoke-Script report "no issues found"?
3 participants