Some optimizations ( type safe, avoid unnecessary memory manage, etc...). #70
Conversation
Release 0.0.2
Release 0.1.0
Release 0.1.1
Release 0.1.2
Release 0.2.0
Release 0.3.0
Release 0.3.1
Release 0.4.0
Release 0.5.0
Release 0.6.0
Release 0.7.0
Release 0.7.1
Release 0.8.0
Release 0.9.0
Release 0.10.0
Release 0.10.1
Release 0.11.0
Release 0.12.0
Release 0.13.0
Release 0.14.0
Release 0.15.0
Release 0.16.0
Release 0.17.0
Release 0.18.0
Release 0.19.0
Release 0.20.0
Release 0.21.0
9ce840e to
d831939
Compare
There was a problem hiding this comment.
thank you for this massive PR! some really great changes in here and I've left comments about splitting these up into smaller PRs - that way its easier to test the functionality and we can revert commits if needed.
the places I haven't left comments are for 2 reasons:
- I agree with the non-controversial typing changes (i.e. places where there are no breaking changes). this could probably be its own PR where type safety is improved.
- I'd like to see some of the breaking changes in their own PR once all the other non-controversial changes have been pulled into their own PRs. If you could also point me to any documentation about performance improvements from
RefCountedthat would be great. The change fromNodetoRefCountedwould cause a lot of breaking changes and it would be nice to have the confidence to say its worth it.
thanks again and please let me know if you have any questions
| @@ -1,4 +1,5 @@ | |||
| class_name ChannelsAPI extends TaloAPI | |||
There was a problem hiding this comment.
will use this file as a base for most of my comments:
- I'd like to keep using the
class_name xAPI extends TaloAPIformat - unless there's any reason not to. I like the declarative nature of showing exactly what the class name is. - For consistency I'd prefer if we didn't use the
p_prefixes on variable names. I understand the idea behind it but I don't think the values are being shadowed in the same way they would be in other languages. - Really love that you added these signals - would be great if the changes in this file could be pulled out into their own PR
- Similarly love the idea of the ChannelPage struct - if this could be part of the same PR as the above that would be great!
There was a problem hiding this comment.
- I think a plugin should avoid expose unnecessary class name,
class_nameis global scope, it will contaminate user's namespace. Anyclass_namein plugin should be thought twice.
If you definitely want to keep them has an explicit class name, we can consider to user an independence script with aclass_nameand declare other scripts as a constant value like in "talo_manager.gd" in this pre.
For example:
# talo.gd
class_name TaloScope # Maybe "TaloScope" is not an appropriate name of Talo namespace.
const ChannelsAPI = preload("apis/channels_api.gd")
...
# talo_manager.gd
...
var channels : TaloScope.ChannelsAPI
...
- This is mainly to improve maintainability, for example:
var name: String = "abc"
# The argument "name" is shadowing the property "name".
func foo(name: String) -> void:
var a := name
...... # A very long piece of code.
print(name) # Will print the value of argument "name". (Correct)
If we consider to change the argument "name" and fix relate logic partially, it will be like this:
var name: String = "abc"
# Change "name" to be an int value "id".
func foo(id: int) -> void:
var a := id
...... # A very long piece of code.
# Because "name" is a property in this script, it will not be an error and easy to be ignored.
print(name) # Will print "abc". (Wrong)
There was a problem hiding this comment.
I fully see your point and agree in principle but I think the main problem with polluting namespaces is naming collisions. I'm hopeful that one day gdscript will add namespaces but until then I think 99.9% of Godot projects won't have any naming collisions with any of the Talo APIs
There was a problem hiding this comment.
We can’t assume the user's naming preferences. For example, TimeUtils, it is entirely possible for the user to create type with the same name.
At least we need to add the prefix "Talo" to all Talo types like the C language library.
We can consider it from another perspective, reduce expose unnecessary types to users can reduce the number of related types in the completion list when users type "Talo" in code editor, so user can ignore them. This reduces unnecessary complexity and keep the simplicity of the API.
Especially for some types in the "utilis" folder, they are completely inaccessible to users but have a global class name, this is completely inappropriate.
There was a problem hiding this comment.
yeah I think adding Talo as a prefix to the APIs is a good middle ground, especially because that won't lead to a breaking change
You can refer this document for more details. |
|
I will try to split this pr late. |
|
An another naming issue: But this change quite huge, I'm not sure that it is worth or not. What do you think? |
I personally don't think so. The compiler throws an error if you miss an await with an async function: |
|
Just wanted to say thank you very much for your hard work here - I really appreciate it. I've left some comments on the other PRs and will have a look at the remaining PRs tomorrow. Also very interested to read the multiplayer peer PR, would love to get an explanation from you about how your envision this feature working. |
I prefer to type a correct code without waiting compiler checking. From another perspective, for example: If we have a "_async" suffix, users can know that this is an asynchronous method form its name, user can avoid this issue to reduce time to debug or refer document. Anyway, this just a suggestion. |
Please discuss in #85 . |
|
Hey @Daylily-Zeleen I'm gonna close this PR since most of the changes are now merged via other PRs |
RefCountedto replaceNodeto avoid unnecessary memory manage for users.RefCountedand not conflict withNode.namenow).settings.cfgwhen plugin enableing instead of first time of running game.player_joined,player_left,channel_ownership_transferred,channels_deleted.This is my first contribute to this repo. I'm not sure the changes in this pr is appropriate or not.
If this pr can be merged, the document should do some change, too.
Edit: Except for type safe and
class_namechanges, everything else have been separated into independent PR.