Skip to content
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

Remove OS, GDScript and Object from the list of builtins in syntax highlighting #739

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Oct 19, 2024

OS is a singleton like any other, not a special keyword in GDScript. GDScript is a standard class, not a special keyword either. Object is a Variant type, but it's also a nullable type that is highlighted as a standard object in the Godot script editor.

@DaelonSuzuka
Copy link
Collaborator

@Calinou this is ready to be rebased. (I'm not smart enough to use the GitHub merge editor)

…tax highlighting

OS is a singleton like any other, not a special keyword in GDScript.
GDScript is a standard class, not a special keyword either.
Object is a Variant type, but it's also a nullable type that is
highlighted as a standard object in the Godot script editor.
@Calinou Calinou force-pushed the syntax-highlighting-os-not-builtin branch from d1fbfcb to 7bf83b3 Compare October 21, 2024 12:06
@DaelonSuzuka DaelonSuzuka merged commit c33982d into godotengine:master Oct 21, 2024
4 checks passed
@@ -437,7 +437,7 @@
}
},
"builtin_classes": {
"match": "(?<![^.]\\.|:)\\b(OS|GDScript|Vector2|Vector2i|Vector3|Vector3i|Vector4|Vector4i|Color|Rect2|Rect2i|Array|Basis|Dictionary|Plane|Quat|RID|Rect3|Transform|Transform2D|Transform3D|AABB|String|Color|NodePath|Object|PoolByteArray|PoolIntArray|PoolRealArray|PoolStringArray|PoolVector2Array|PoolVector3Array|PoolColorArray|bool|int|float|Signal|Callable|StringName|Quaternion|Projection|PackedByteArray|PackedInt32Array|PackedInt64Array|PackedFloat32Array|PackedFloat64Array|PackedStringArray|PackedVector2Array|PackedVector2iArray|PackedVector3Array|PackedVector3iArray|PackedVector4Array|PackedColorArray|super)\\b",
"match": "(?<![^.]\\.|:)\\b(Vector2|Vector2i|Vector3|Vector3i|Vector4|Vector4i|Color|Rect2|Rect2i|Array|Basis|Dictionary|Plane|Quat|RID|Rect3|Transform|Transform2D|Transform3D|AABB|String|Color|NodePath|PoolByteArray|PoolIntArray|PoolRealArray|PoolStringArray|PoolVector2Array|PoolVector3Array|PoolColorArray|bool|int|float|Signal|Callable|StringName|Quaternion|Projection|PackedByteArray|PackedInt32Array|PackedInt64Array|PackedFloat32Array|PackedFloat64Array|PackedStringArray|PackedVector2Array|PackedVector2iArray|PackedVector3Array|PackedVector3iArray|PackedVector4Array|PackedColorArray|super)\\b",
"name": "entity.name.type.class.builtin.gdscript"
},
"const_vars": {
Copy link

@AlfishSoftware AlfishSoftware Oct 21, 2024

Choose a reason for hiding this comment

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

OS is a singleton like any other, not a special keyword in GDScript.

@Calinou OS is unusual, as it's all uppercase even though it's a type/class. Are you sure it won't be matched by const_vars and be colored like a constant (such as NAN, INF)?
If so, maybe it should be added as a special case in pascal_case_class, so it's matched as a type even when semantic coloring is disabled/unavailable (doesn't seem like it's supported currently).

Also, I don't know if it's just OS that breaks the norm, or if there's others (except RID, which is already listed above as a basic type).

Copy link

@AlfishSoftware AlfishSoftware Oct 21, 2024

Choose a reason for hiding this comment

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

Ah OS is definitely not the only one. On a quick look through the class list in stable, there's also these:
IP, JSON, JSONRPC, UPNP, XRVRS

Singletons being colored like a constant is not the worst thing, but at least in the case of JSON and UPNP, they can be constructed with new().

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this was why I added OS to this list in the first place.

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.

3 participants