PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using UnityEngine.InputSystem.Haptics;
using Unity.Collections.LowLevel.Unsafe;
using UnityEngine.InputSystem.Controls;
using UnityEngine.InputSystem.Layouts;
using UnityEngine.InputSystem.LowLevel;
using UnityEngine.InputSystem.DualShock;
using UnityEngine.InputSystem.EnhancedTouch;
using UnityEngine.InputSystem.HID;
using UnityEngine.InputSystem.Users;
using UnityEngine.InputSystem.XInput;
using UnityEngine.InputSystem.Utilities;
using Unity.Profiling;

#if UNITY_EDITOR
using UnityEditor;
using UnityEngine.InputSystem.Editor;
using UnityEditor.Networking.PlayerConnection;
#else
using System.Linq;
using UnityEngine.Networking.PlayerConnection;
#endif

#if UNITY_EDITOR
using CustomBindingPathValidator = System.Func<string, System.Action>;
#endif

namespace UnityEngine.InputSystem
{
    /// <summary>
    /// This is the central hub for the input system.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Text;
using Unity.Collections;
using UnityEngine.InputSystem.Composites;
using UnityEngine.InputSystem.Controls;
using Unity.Collections.LowLevel.Unsafe;
using UnityEngine.InputSystem.LowLevel;
using UnityEngine.InputSystem.Processors;
using UnityEngine.InputSystem.Interactions;
using UnityEngine.InputSystem.Utilities;
using UnityEngine.InputSystem.Layouts;
using Unity.Profiling;
using UnityEngineInternal.Input;
#if UNITY_6000_5_OR_NEWER
using UnityEngine.Assemblies;
#endif

#if UNITY_EDITOR
using UnityEngine.InputSystem.Editor;
#endif

#if UNITY_EDITOR
using CustomBindingPathValidator = System.Func<string, System.Action>;
#endif

#pragma warning disable CS0649
namespace UnityEngine.InputSystem
{
    using DeviceChangeListener = Action<InputDevice, InputDeviceChange>;

using System;
using System.Runtime.CompilerServices;
using Unity.Collections.LowLevel.Unsafe;
using UnityEngine.InputSystem.Utilities;

namespace UnityEngine.InputSystem.LowLevel
{
    /// <summary>
    /// Information about a memory region storing input state.

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

PR Code Suggestions ✨

No code suggestions found for the PR.

Codecov Report

All modified and coverable lines are covered by tests ✅

@@           Coverage Diff            @@
##           develop    #2316   +/-   ##
========================================
  Coverage    77.95%   77.95%           
========================================
  Files          476      476           
  Lines        97443    97445    +2     
========================================
+ Hits         75961    75963    +2     
  Misses       21482    21482           

Flags with carried forward coverage won't be shown. Click here to find out more.

... and 1 file with indirect coverage changes

Although this ticket cleans up code I agree with AI review that it removes/reduces ability to track/trace these issues down going forward. I would strongly suggest that at least each TODO, REVIEW etc provides traceability back to its origin. Just looking at the tickets without the code contexts makes it very difficult to decipher IMO. Hence, would it be possible to link each removed entry back to the exact line where it was removed for reference (from the JIRA ticket), and maybe also link back to this PR (the PR removing it) to make it easier to review the removal in context? What do you think?

I also believe there is merit to the AI review on documentation gap where it may either be handled as being defined as part of the filed JIRA ticket as documentation debt or addressed directly on this PR. E.g. removing something like "TODO: Make this method thread-safe" could be argued it should result in documentation of such method to explicitly state the method is not thread-safe unless the Unity documentation convention is that methods are by default not thread-safe and only thread-safe if explicitly stated. I am not aware whether such a convention exists but its a fair point IMO.

The request changes maps to the traceability aspect of linked tickets and not to the code itself.

The request changes maps to the traceability aspect of linked tickets and not to the code itself.

Done, added 2 links in each Jira ticket to ensure each removed todo can be traced back to its original source and intent.
• A link to the exact line/location where it was removed.
• A link to the PR that removed it, so reviewers can see the change in context.

Just my opinion warning😅: I don't really see a real benefit and to me it just seems like hiding things under the carpet. Wouldn't it be more likely to be implemented if someone is working on the area -> sees a todo -> thinks about it as it is within their context and or implements it right away. As opposed to: works on the area -> a ticket about the todo exists in some epic that nobody reads -> maybe in a blue moon we work on that epic.

But I'm absolutely not blocking it if you feel like It's better this way

I will create an EPIC JIRA so we can have some guarantees that they don't get forgotten forever.
A more active approach would be to ask people to read them and then set a meeting to review each point.

I don't think these should be removed from code if they are not being fixed. Having them in JIRA tickets it's not a better system because developers will not browse JIRA if they need context on a particular issue a class.