Skip to content

Conversation

@ckaiser
Copy link
Contributor

@ckaiser ckaiser commented May 18, 2024

I noticed that signal parameters got highlighted as class names if they matched, so it could potentially be confusing to see a signal declared as signal name(bool), since at first glance it might seem like it accepts only booleans, when in fact that's just the parameter name. I also added a warning when any signal parameters shadow global class names, to discourage it.

This led me down a bit of a rabbit hole with regards to syntax highlighting for function parameters in general, which suffered from the same problem, but the fix was similar to the one for signals except lambdas which was a bit more complicated since those were not being flagged properly when there was a space after the func token.

I attempted a fix for that as well, although I'm not super familiar with this kind of code so there may be a cleaner fix.

Before:
signal_highlight_before

After:
signal_highlight_after

@ckaiser ckaiser requested a review from a team as a code owner May 18, 2024 16:24
@AThousandShips AThousandShips added this to the 4.x milestone May 18, 2024
@ckaiser ckaiser force-pushed the signal-type-highlighting branch from 3a7324d to 229011f Compare August 11, 2024 19:13
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 39fc116), it works as expected.

Before After
image image
Code used in the screenshots
extends Node2D

signal _int(int)
signal _int2(int: int)

var _bool: bool = false


func bool(_bool: bool) -> bool:
	_int.emit(1)
	_int2.emit(2)
	return _bool


func _ready() -> void:
	var _float = func float(_float: float) -> float: return _float
	bool(_bool)

@ckaiser ckaiser force-pushed the signal-type-highlighting branch from 229011f to 8cb3db2 Compare August 13, 2024 04:59
@ckaiser ckaiser force-pushed the signal-type-highlighting branch from 8cb3db2 to b6a49c5 Compare September 2, 2024 06:13
@ckaiser ckaiser force-pushed the signal-type-highlighting branch from b6a49c5 to 630bf44 Compare January 11, 2025 22:04
@ckaiser ckaiser force-pushed the signal-type-highlighting branch from 630bf44 to a085c5e Compare April 1, 2025 00:47
@leandro-benedet-garcia
Copy link
Contributor

I would think it's better to get an exception for trying to override a built in type, tho?

@Mickeon
Copy link
Member

Mickeon commented Apr 7, 2025

I would think it's better to get an exception for trying to override a built in type, tho?

The PR showcases and addresses an edge-case in the highlighter's logic which is potentially confusing, especially for beginners. Whether an error should be printed is not relevant (Although at most it should be a warning)

@ckaiser ckaiser force-pushed the signal-type-highlighting branch from a085c5e to fb47195 Compare May 27, 2025 00:49
@Repiteo Repiteo modified the milestones: 4.x, 4.6 Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants