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

Getting ImageResizer code base with code analysis #1041

Closed
10 tasks done
crutkas opened this issue Jan 3, 2020 · 14 comments
Closed
10 tasks done

Getting ImageResizer code base with code analysis #1041

crutkas opened this issue Jan 3, 2020 · 14 comments
Assignees
Labels
Product-Image Resizer Things regarding image resizing module

Comments

@crutkas
Copy link
Member

crutkas commented Jan 3, 2020

https://github.com/microsoft/PowerToys/tree/dev/imageResizer

  • Upgrade to .NET 4.7.2 to match existing code base on ImageResizer
  • Upgrade to .NET 4.7.2 to match existing code base on ImageResizerTest
  • Add StyleCop on ImageResizer
  • Add StyleCop on ImageResizerTest
  • Add Code analysis with ruleset.set on ImageResizer w/ globalsupression.cs
  • Add Code analysis with ruleset.set on ImageResizerTest w/ globalsupression.cs
  • Fix all warnings and messages
  • Add clangformat from main repo on ShellExtensions
  • all unit tests pass
  • 🤘🤘
@crutkas
Copy link
Member Author

crutkas commented Jan 3, 2020

@ryanbodrug-microsoft i did a push, think i got all warnings, however the tests don't want to seem to actually run.

we also should mimic the clang stuff we're doing in the main repo

assign to whomever.

@crutkas
Copy link
Member Author

crutkas commented Jan 3, 2020

image

@crutkas crutkas added the Product-Image Resizer Things regarding image resizing module label Jan 3, 2020
@crutkas
Copy link
Member Author

crutkas commented Jan 3, 2020

xref #53

@arjunbalgovind
Copy link
Contributor

arjunbalgovind commented Jan 3, 2020

@crutkas It looks like the unit tests weren't running as the xunit.runner.visualstudio NuGet package wasn't added to the project dependencies, and Visual Studio doesn't run xunit tests unless this adapter is installed. Adding it results in all the tests passing on the initial version of the code. I'll verify if they pass after the changes that were made above.

Edit: They pass in the latest version as well if the same change is made. Should I push the changes in the .csproj file to this branch directly or through a pull request?

@crutkas
Copy link
Member Author

crutkas commented Jan 3, 2020

Just check it in to that branch right now. We will do a more formal review for the actual module integration

@arjunbalgovind arjunbalgovind self-assigned this Jan 6, 2020
@arjunbalgovind
Copy link
Contributor

@crutkas The clangformat file gets automatically detected by Visual Studio if it is in the same git source tree. I've formatted the files in ShellExtensions using the clang file. All unit tests pass as well.
There are still a couple of warnings on the C++ side but they seem to be related to "Return value ignored" in some LoadString function calls and "enum type unscoped" for some enum type which is defined in a library header file, so it can't be fixed. If these warnings can be left as is then this issue can probably be closed.

@crutkas
Copy link
Member Author

crutkas commented Jan 6, 2020

@arjunbalgovind is there a way to suppress them with that reasoning? I know in c# you can for code analysis

@crutkas
Copy link
Member Author

crutkas commented Jan 6, 2020

  • do that then close :)

@arjunbalgovind
Copy link
Contributor

arjunbalgovind commented Jan 6, 2020

Suppressed those warnings using the #pragma warning(suppress) directive, which suppresses it for one line of code.
Closing the issue as all the points are completed.
🤘🤘

@crutkas
Copy link
Member Author

crutkas commented Jan 6, 2020

Where is the associated PR?

@crutkas crutkas reopened this Jan 6, 2020
@arjunbalgovind
Copy link
Contributor

@crutkas I checked in the changes directly to the dev/imageResizer branch. Should I revert my last few commits and add it as a PR to this branch instead?

@crutkas
Copy link
Member Author

crutkas commented Jan 7, 2020

you should be in the imageResizer branch, i just expected work to come in as a PR so the work has review. If Ryan or someone reviewed it, I'm good.

@crutkas crutkas closed this as completed Jan 7, 2020
@ryanbodrug-microsoft
Copy link
Contributor

ryanbodrug-microsoft commented Jan 7, 2020 via email

@arjunbalgovind
Copy link
Contributor

Discussed with Ryan, he will be reviewing the changes offline. I'll make sure to use PRs for all future changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-Image Resizer Things regarding image resizing module
Projects
None yet
Development

No branches or pull requests

3 participants