-
Notifications
You must be signed in to change notification settings - Fork 328
FIX: Change type aliasing to use the type implementing static interface instead of using derived type. #2265
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,6 @@ | |
| using UnityEngine; | ||
| using UnityEngine.InputSystem; | ||
| using UnityEngine.TestTools; | ||
| using Is = UnityEngine.TestTools.Constraints.Is; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was this not being used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think when UnityEngine.TestTools.Constraints isn't included (with using) there is no ambiguous behavior that needs explicit aliasing to be resolved, but I probably need to double check with a version that enables this code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was no GC memory allocation check so UnityEngine.TestTools.Constraints isn't needed at least There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using NUnit.Framework; should be enough There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for checking! |
||
|
|
||
| partial class CoreTests | ||
| { | ||
|
|
||
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.
Suggestion: Changing the alias for
IsfromUnityEngine.TestTools.Constraints.Isto the baseNUnit.Framework.Ismay cause build errors. The derived type contains Unity-specific constraints likeIs.AllocatingGCMemory. Please verify that no tests in this file use such constraints, as they will no longer be accessible through theIsalias. [possible issue, importance: 7]🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent
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.
Yes, but that is a very specific use case that is still supported since the Unity extension is based on the same class being aliased here. Not using the base is considered bad practise by e.g. JetBrains Rider.
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.
UnityEngine.TestTools.Constraints just need a using statement to make it accessible.