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

Enforce whitespace in ruleset #4138

Merged
merged 3 commits into from
Aug 26, 2021
Merged

Conversation

nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Jul 7, 2021

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/1041

Regression? Last working version:

Description

This is meant to complement the dotnet format check we have in the CI.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@nkolev92 nkolev92 requested a review from a team as a code owner July 7, 2021 00:50
@@ -13,6 +13,7 @@
<Shipping>true</Shipping>
<XPLATProject>true</XPLATProject>
<UsePublicApiAnalyzer>perTfm</UsePublicApiAnalyzer>
<WarningsNotAsErrors>$(WarningsNotAsErrors);IDE0055</WarningsNotAsErrors>
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason why these warnings are suppressed in this project is the fact that we're using submodules and not the packages, NuGet/Home#2345, so we don't have control over all of the code.

Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

Trailing whitespace at the end of a line annoys me much more than it should, so I completely approve of this PR 👍

@nkolev92
Copy link
Member Author

nkolev92 commented Jul 9, 2021

Something's off with the apex machines :/

It suggests that it's usually an issue due to the analyzers not being in sync with the tooling version, but I haven't noticed that being the case.

After all, E2E tests run on the same machines as well.

@zivkan
Copy link
Member

zivkan commented Jul 12, 2021

E2E job doesn't compile anything, it downloads artifacts created by the build + unit test job, whereas Apex job compiles the test binary, in addition to running the tests.

I noticed that it's using VS 2019 to compile the test binaries, and using the latest .net 6 preview that VS 2022 installs. Perhaps the .net 6 SDK is no longer compatible with msbuild 16, and the msbuild/.net sdk teams didn't set up the SDK resolver correctly to account for this? Perhaps worth checking with them. You can also try locally, to confirm if you get the same behaviour on your machine. Maybe we need up to update VS 2022 (& .net 6 sdk) on the CI machines.

@nkolev92
Copy link
Member Author

That's great info thanks @zivkan

I'll try to investigate that soon.

@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 20, 2021
@ghost
Copy link

ghost commented Jul 20, 2021

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@nkolev92 nkolev92 force-pushed the dev-nkolev92-enforceWhitespaceInVS branch from 49938aa to 689c3b8 Compare July 20, 2021 19:54
@ghost ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 20, 2021
@ghost
Copy link

ghost commented Jul 27, 2021

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 27, 2021
@nkolev92 nkolev92 force-pushed the dev-nkolev92-enforceWhitespaceInVS branch from 689c3b8 to 1137477 Compare August 2, 2021 19:34
@ghost ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 2, 2021
@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 9, 2021
@ghost
Copy link

ghost commented Aug 9, 2021

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@ghost ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 14, 2021
@nkolev92 nkolev92 force-pushed the dev-nkolev92-enforceWhitespaceInVS branch 2 times, most recently from 778eb75 to d7489c4 Compare August 16, 2021 21:44
@nkolev92 nkolev92 force-pushed the dev-nkolev92-enforceWhitespaceInVS branch from d7489c4 to 2e4e0c6 Compare August 23, 2021 21:39
@nkolev92 nkolev92 force-pushed the dev-nkolev92-enforceWhitespaceInVS branch from b648052 to 9d01d07 Compare August 25, 2021 18:12
@nkolev92 nkolev92 merged commit d888411 into dev Aug 26, 2021
@nkolev92 nkolev92 deleted the dev-nkolev92-enforceWhitespaceInVS branch August 26, 2021 17:22
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.

4 participants