Removed redundant variable assignment in remote debugger

[TheRealChestnut]

Bugsquad edit: Fixes #50848.

[TheRealChestnut]

I'm very new to GitHub and contributing to open source projects so if I've done something stupid please let me know :)

[YuriSizov]

The message needs to be amended to reflect the nature of change, not the line that you've touched. Something like "Remove a redundant assignment in the remote debugger" should suffice.

[akien-mga]

This change preserves what was the current behavior, but I'm not sure that it's correct.

The intent of this code's author was that in the error case, arr[i + max] should be Variant(), not a non-numeral value. So this is probably what needs to be changed (with an else instead of removing the assignment).

CC @simpuid who wrote that code, and @Faless who reviewed.

[Faless]

Yeah, that seems to be the case. It slipped my review. Could also be arr[i + max] = 0 instead of Variant().

[simpuid]

This is the part where we are getting the float back from Variant
@Faless Should we add an extra check there too?

[Faless]

Sorry, I missed this comment, yes, the cast should be automatic and not fail, but better be safe, so I would add the check (setting the value to 0 is not is_num).

[akien-mga]

Congrats for your first contribution :) I left a comment on whether this fix is the correct one, we might need input from contributors who know about it to confirm.

Aside from this, on the workflow side, we usually want PRs with a self-contained change like this to only contain one commit, so that our Git history stays readable and useful (that's also why we require explicit commit messages so that we can see what a commit does at a glance - you already improved it, see CONTRIBUTING.md for more details for future contributions). For this PR, that means that you should squash the current 3 commits into one using the procedure described in PR workflow.

[TheRealChestnut]

Thanks for all of the help! I'll squash the commits and leave the rest of the request as it is (as I am assuming it is bad practice to push further changes mid review)

[akien-mga]

I'll squash the commits and leave the rest of the request as it is (as I am assuming it is bad practice to push further changes mid review)

Based on the current feedback you can already change the code. The correct fix would be something like:

if (!monitor_value.is_num()) {
	ERR_PRINT("Value of custom monitor '" + String(custom_monitor_names[i]) + "' is not a number");
	arr[i + max] = 0;
} else {
	arr[i + max] = monitor_value;
}

[TheRealChestnut]

Thanks for letting me know. I'll go ahead and change that now

[akien-mga]

Your latest push added a merge commit which is not relevant for the merged history, you can remove it by rebasing on the master branch.

[TheRealChestnut]

Really sorry about that - something went very wrong when I tried to remove the previous commit.

[YuriSizov]

Sorry for the long pause. You've made the correct change, but we failed to approve it in a timely manner. In the meantime, this was fixed by #59911 alongside other potential code issues. Thanks anyway!