-
Notifications
You must be signed in to change notification settings - Fork 59
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
Ignore warning CS8002 when building in debug mode #251
Ignore warning CS8002 when building in debug mode #251
Conversation
@Eneuman Thanks for your PR! Here are review comments for diff --git a/src/EndpointManagerLauncher/endpointmanagerlauncher.csproj b/src/EndpointManagerLauncher/endpointmanagerlauncher.csproj: My in-depth review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. Additionally, NoWarns for rules 1701;1702 are being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look correct and the pull request is properly linked to the issue it is addressing. My only suggestion would be to add a comment to the code explaining why the NoWarns are being added, so that future developers can understand the reasoning behind the change. Here are review comments for diff --git a/src/LocalAgent/LocalAgent.csproj b/src/LocalAgent/LocalAgent.csproj: My in-depth review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. Additionally, NoWarns for rules 1701;1702 are being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look good and are properly documented. I suggest double-checking the NoWarns to make sure they are properly applied to the correct configurations. Additionally, I suggest adding a comment to the code to explain why the NoWarns are being added, as this will help other developers understand the purpose of the changes. Here are review comments for diff --git a/src/common.tests/common.tests.csproj b/src/common.tests/common.tests.csproj: My review: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it can be safely ignored in debug builds. NoWarns for rules 1701;1702 are being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look good and I don't see any problems. However, I suggest that you add a comment to the code to explain why the NoWarns are being added, so that it is easier to understand in the future. Here are review comments for diff --git a/src/common/common.csproj b/src/common/common.csproj: My in-depth review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. Additionally, NoWarns for rules 1701 and 1702 are being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look correct and should resolve the issue. However, I suggest adding a comment to the code to explain why the NoWarns are being added, as this will help other developers understand the code better. Additionally, I suggest adding a unit test to ensure that the NoWarns are being applied correctly. This will help ensure that the code is working as expected. Here are review comments for diff --git a/src/devhostAgent.restorationjob.tests/devhostAgent.restorationjob.tests.csproj b/src/devhostAgent.restorationjob.tests/devhostAgent.restorationjob.tests.csproj: My review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. The NoWarns for rules 1701;1702 are also being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look correct and should not cause any issues. However, I would suggest adding a comment to the code to explain why the NoWarns are being added, as this will help other developers understand the purpose of the changes. Additionally, I would suggest adding a unit test to ensure that the changes are working as expected. Here are review comments for diff --git a/src/devhostagent.restorationjob/devhostAgent.restorationjob.csproj b/src/devhostagent.restorationjob/devhostAgent.restorationjob.csproj: My review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. Additionally, NoWarns for rules 1701 and 1702 are being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look correct and should resolve the issue. However, I suggest adding a comment to the code to explain why the NoWarns are being added and why the rule CS8002 is being ignored in debug mode. This will help future developers understand the code and make it easier to maintain. Here are review comments for diff --git a/src/devhostagent/devhostAgent.csproj b/src/devhostagent/devhostAgent.csproj: My review of this pull request is that it looks like a good solution to the problem of ignoring the warning CS8002 when building in debug mode. The body of the pull request explains why the warning can be safely ignored in debug builds and why NoWarns for rules 1701 and 1702 are being added. The changes look correct and the pull request resolves the issue #250. My only suggestion would be to add a comment to the changes to explain why the NoWarns are being added. This would help other developers understand the purpose of the changes and make it easier to maintain the code in the future. Here are review comments for diff --git a/src/dsc.tests/dsc.tests.csproj b/src/dsc.tests/dsc.tests.csproj: My review of this pull request is that it looks like a good solution to the problem of ignoring the warning CS8002 when building in debug mode. The body of the pull request explains why the warning can be safely ignored in debug builds, and the changes show that the NoWarns for rules 1701;1702 are being added to the project. The pull request also fixes issue #250. My only suggestion would be to add a comment to the changes to explain why the NoWarns for rules 1701;1702 are being added. This would help other developers understand the purpose of the changes and make it easier to review the pull request. Here are review comments for diff --git a/src/dsc/dsc.csproj b/src/dsc/dsc.csproj: My review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. The NoWarns for rules 1701 and 1702 are also being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The only suggestion I have is to make sure that the changes are tested thoroughly before merging. It is important to make sure that the changes do not cause any unexpected issues or break any existing functionality. Here are review comments for diff --git a/src/endpointmanager.tests/endpointmanager.tests.csproj b/src/endpointmanager.tests/endpointmanager.tests.csproj: My in-depth review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. The NoWarns for rules 1701;1702 are also being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The only suggestion I have is to make sure that the changes are tested thoroughly before merging the pull request. This will ensure that the changes are working as expected and that no other issues have been introduced. Here are review comments for diff --git a/src/endpointmanager/endpointmanager.csproj b/src/endpointmanager/endpointmanager.csproj: My in-depth review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. Additionally, NoWarns for rules 1701 and 1702 are being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look correct and should not cause any issues. However, I suggest adding a comment to the code to explain why the NoWarns are being added and why the rule CS8002 is being ignored in debug mode. This will help future developers understand the code and make it easier to maintain. Here are review comments for diff --git a/src/library.tests/library.tests.csproj b/src/library.tests/library.tests.csproj: My in-depth review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. The NoWarns for rules 1701;1702 are also being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The only suggestion I have is to make sure that the changes are tested thoroughly before merging the pull request. This will ensure that the changes are working as expected and that no other issues have been introduced. Here are review comments for diff --git a/src/library/library.csproj b/src/library/library.csproj: My review: This pull request looks good and resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. Additionally, NoWarns for rules 1701 and 1702 are being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look correct and the pull request should be accepted. However, I suggest adding a comment to the code to explain why the NoWarns for rules 1701 and 1702 are being added, as this may not be immediately clear to other developers. Additionally, I suggest adding a comment to the pull request to explain why the rule CS8002 is being ignored in debug builds. Here are review comments for diff --git a/src/routingmanager.tests/routingmanager.tests.csproj b/src/routingmanager.tests/routingmanager.tests.csproj: My in-depth review of this pull request is as follows: This pull request looks to be addressing the issue of the rule CS8002 ("Assembly does not have a strong name") occurring when signing is enabled on the projects. The proposed solution is to ignore the rule CS8002 when building in debug mode, and to add NoWarns for rules 1701 and 1702 as they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes proposed in this pull request appear to be valid and should address the issue. However, I would suggest that the NoWarns for rules 1701 and 1702 should be added to both the debug and release builds, as this will ensure that the warnings are ignored in both cases. Additionally, I would suggest that the pull request should include a comment explaining why the NoWarns are being added, as this will help other developers understand the purpose of the changes. Here are review comments for diff --git a/src/routingmanager/routingmanager.csproj b/src/routingmanager/routingmanager.csproj: My in-depth review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. Additionally, NoWarns for rules 1701 and 1702 are being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look correct and the pull request is properly linked to the issue it is addressing. My only suggestion would be to add a comment to the code explaining why the NoWarns are being added, so that future developers can understand the reasoning behind the change. Here are review comments for diff --git a/src/testhelpers/testhelpers.csproj b/src/testhelpers/testhelpers.csproj: My review of this pull request is as follows: This pull request looks good overall. It resolves 26 warnings by ignoring the rule CS8002 when building in debug mode. The rule CS8002 ("Assembly does not have a strong name") occurs because signing is enabled on the projects, and signing will be done in release build so it is safe to ignore this warning in debug builds. Additionally, NoWarns for rules 1701;1702 are being added because they are default ignored in certain projects and when overriding the default behavior they get automatically added. The changes look good and I don't see any problems with them. However, I would suggest adding a comment to the code to explain why the NoWarns are being added, as this will help other developers understand the purpose of the changes. |
src/devhostagent.restorationjob/devhostAgent.restorationjob.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good except the empty NO warn without codes
and left a minor comment.
I also removed the empty . |
…b.com/Eneuman/Bridge-To-Kubernetes into feature/ignore-cs8002-in-debug-builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good.
This PR resolved 26 warnings by ignores the rule CS8002 when building in debug mode.
The rule CS8002 ("Assembly does not have a strong name") occures because signing is enabled on the projects.
Signing will be done in release build so I think this warning can be safely ignored in debug builds.
Fixes #250
NoWarns for rules 1701;1702 are beeing added because they are default ignored in certain projects and when overriding the default behavior they get automaticly added.