-
-
Notifications
You must be signed in to change notification settings - Fork 20.9k
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
Add a keyword for abstract classes in GDScript #67777
base: master
Are you sure you want to change the base?
Conversation
845d75c
to
f7f27ce
Compare
8daace1
to
36a62f0
Compare
36a62f0
to
3071b44
Compare
3071b44
to
74670c0
Compare
74670c0
to
955143a
Compare
Are abstract methods supported with this PR? |
@ryanabx No, only abstract classes are supported with this PR. |
955143a
to
2d4113e
Compare
2d4113e
to
b0390a3
Compare
|
Talked about in the current GDScript meeting. The PR should implement the elements 1 and 2 stated by @dalexeev. For number 3, we're still discussing this in the meeting, so no decision has been made. |
Yeah, my thoughts on 3. are whether it makes sense to (for this PR or eventually) have the "granularity" of abstractness exist at the class or at the method level. For example, with it at the method level: class AbstractBecauseOfAbstractMethods:
func this_method_is_implemented():
pass
@abstract
func this_method_is_virtual_slash_abstract()
func this_is_also_implemented():
pass
# ----------------------
class StillAbstract extends AbstractBecauseOfAbstractMethods:
func some_new_implemented_func():
pass
# ----------------------
class FinallyNotAbstractClass extends StillAbstract:
func this_method_is_virtual_slash_abstract():
pass We could force the users to annotate a class to be marked as abstract if any of its functions are still abstract. This would give people more granularity in what precisely makes the class abstract. It doesn't make GDScript any more expressive, since you can always 1) tag a class as abstract, then 2) have methods implemented with Folks agreed during the meeting that this wouldn't be a blocking thing for this PR, and that it could be implemented later :) |
I will get to work on 1. For 2, this is trivial because it's just removing |
@aaronfranke For me, this is more of a language feature, since it prohibits instantiation of the class, and not only makes the class unselectable in the editor dialog. Especially if we add abstract methods later (#82987). That is, |
dff2288
to
3e95549
Compare
@dalexeev I would say This PR initially did not have It does not make sense to me for |
To support @aaronfranke opinion, global |
Yeah that's true, if the behavior is significantly different with abstract methods a keyword might make more sense. |
0716b68
to
f726965
Compare
@@ -0,0 +1,2 @@ | |||
@abstract | |||
class_name TestConstructAbstractScriptNotest |
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.
The test class look too simple for a real abstract class.
Only mark a class as abstract will not cover a typical abstract class.
An abstract class has typical full implemented functions and also abstract function you need to implement in the inheriting class.
What does an abstract function signature look like?
@abstract
class_name TestConstructAbstractScriptNotest extends Node:
func foo() -> String:
return "foo"
# define a abstract function you need to implement by using the annotation
@abstract
func bar() ->String;
# or define a abstract function by using the key word
abstract func bar() ->String;
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.
This PR does not implement abstract functions, only abstract classes. See #82987
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.
a there is a follow-up 👍
I'm confused on what we're waiting on for this to be merged. Are we just waiting for people on the gdscript team to review this? Is there something I can do to help? |
You can help get this PR merged by testing it locally and making it sure it works correctly (make sure to report results). We can't guarantee this PR will be merged for 4.3 though, since it has the |
There's ongoing discussion among GDScript contributors on whether this should be an annotation or a keyword. We'll update when a consensus is reached. |
At today's GDScript team meeting, we reached a consensus that @aaronfranke Please let us know if you would like to continue working on this and if you need any help. You can discuss implementation details on the |
Needs to be re-implemented as a keyword.
f726965
to
d779831
Compare
Can this be moved to the 4.4 milestone? |
d779831
to
6896f1f
Compare
6896f1f
to
fbebf7a
Compare
fbebf7a
to
8490bc7
Compare
Have had this PR in my fork for over a month, had no issues with it. |
I don't think the name Could someone link me to the previous version were virtual was discarded? |
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.
Looking good to me, havent had issues with this PR at all inside my own fork.
Co-authored-by: Danil Alexeev <[email protected]>
8490bc7
to
84e20dc
Compare
Implements and closes godotengine/godot-proposals#5641
This PR adds a keyword for marking a script class as abstract in GDScript.
I tested multiple combinations of abstract and non-abstract inheritance and they all work. As an example, in this image
MyAbstract
isabstract
, while the other is not. Note that ifExtendsMyAbstract
was made abstract then both would be hidden.And here's what happens if you try to instance with
.new()
:A previous version of the PR used
@virtual
, but the feedback has been overwhelming thatabstract
is better. After that, another previous version of this PR used an annotation@abstract
, but it was discussed that a keyword is preferred.Production edit: closes godotengine/godot-roadmap#66