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

Improve static typing support #588

Merged
merged 1 commit into from
May 14, 2024
Merged

Improve static typing support #588

merged 1 commit into from
May 14, 2024

Conversation

mphe
Copy link
Contributor

@mphe mphe commented Apr 6, 2024

I enabled various warnings in debug/gdscript related to enforcing static typing, e.g. unsafe_property_access or unsafe_method_access.
I also treat some of them as errors.

I've just wrote a unit test and immediately ran into errors because Gut doesn't seem to make much use of static typing.
This patch gives the gut object a class_name and adds an explicit return type to the get_elapsed_time function.

I'm not sure if GutContext is a sensible name for this class, I'm open for suggestions.

This patch just contains fixes for the parts that immediately caused errors in my workflow.
There might be more PRs to come in the future if I run into trouble again :)

@mphe mphe changed the title Add type hints where user interaction is likely Improve static typing support Apr 6, 2024
@bitwes
Copy link
Owner

bitwes commented Apr 22, 2024

Does checking "Exclude Addons" still cause errors? If so, then I might consider these changes. GUT was started long before static typing was widely used and class_name has it's own issues.

@mphe
Copy link
Contributor Author

mphe commented Apr 22, 2024

Yes, because a test file is not an addon, hence you will get errors.

@Zeneixe
Copy link

Zeneixe commented Apr 28, 2024

Just noticed that double(MyScene) will create loads of static type checker errors because the generated code isn't typed correctly, this bypasses the exclude addons option. @bitwes

@bitwes
Copy link
Owner

bitwes commented May 9, 2024

Just noticed that double(MyScene) will create loads of static type checker errors

This should be fixed soon via #601.

@bitwes
Copy link
Owner

bitwes commented May 9, 2024

@mphe I'm still thinking about this. It feels like it is opening a can of worms. With that in mind, I had the horrible idea of moving the directory containing your tests to the addons folder. This feels like a total perversion of the addons folder, but I think it would work, especially with your other PR to auto exclude addons #600.

I would love to hear any opinions about this. I know I would be hesitant to do it in my own project, it feels gross. It might be the best way to workaround the issues you've come across and any future related issues until Godot offers a way to exclude an arbitrary directory.

@mphe
Copy link
Contributor Author

mphe commented May 9, 2024

I think the can of worms is already open and it's time to put the worms back and close the lid.
addons/ is for addons, not for tests. Tests belong to the project. If the project is an addon, the tests will reside somewhere under addons/ naturally, but if the project is not an addon, users shouldn't be forced to move their tests to addons just to workaround internal issues of GUT.

#600 just helps for not having to manually change project settings before running the test suite, it doesn't help with diagnostics in editor.
So if the user has typing related warnings/errors enabled and exclude_addons is false, they also arise when the tests folder is in addons/

Hence, I think it's better to gradually try to fix typing related errors as far as possible.
However, it's fine to suggest users facing related problems to move their test folder to addons/ for a temporary workaround.

@bitwes
Copy link
Owner

bitwes commented May 13, 2024

I'm ok with get_elpased_time having a typed return value. The other change is more difficult, as naming things is gross. After too much deliberation I've settled on GutMain. Below are the other names I've come up with. I would entertain any votes or suggestions, but I'd like to avoid discussing it too much.

Some other gut.gd class_name choices

Cannot Use

  • Gut - too generic and likely to clash with user class names
  • GutRunner - GutRunner is already in use by a scene used to run tests.

Other Choices

_Gut GastrointestinalUpperTract
(G U T...nice)
GutBrains
GutContext GutDigestiveSystem GutExecutor
GutGodotUnitTest GutGut GutGutGut
GutInternal GutMain GutRun
GutRunnerOfTests GutTestExecution GutTestExecutor
GutTestRun GutTestRunner GuttyMcGutFace
McGutPants MuscularHollowOrganInUpperGastrointestinalTract

Best

_Gut GutMain GutContext
GutTestExecution GutExecutor

Unnecessary Author Rant

just to workaround internal issues of GUT

My gut reaction (pun intended) was "GUT works fine, this is at best a usability enhancement, not an internal GUT issue." The more static typing I have to do in a dynamic, interpreted language the more annoyed I become. I would like GUT to be usable by as many people as possible and people like these features. They can also have performance benefits so GUT should try to accomodate this style as much as it can. I will, however, go crazy-go-nuts when "GDTypeScript" is created by some statically typed zealot (one who is a zealot for statically typed features and/or var zealot := ThereWillBeDatatypes.new()).

addons/gut/test.gd Outdated Show resolved Hide resolved
@mphe
Copy link
Contributor Author

mphe commented May 13, 2024

Renamed GutContext -> GutMain, even though I think MuscularHollowOrganInUpperGastrointestinalTract is a much better name.

Regarding your rant:
You're right that "internal issue" is not correct. It's apparently more like a personal preference. So yes, it is a usability enhancement, but also a usability requirement for people that configured related warnings to be treated as errors, which is an absolutely sane decision, especially in larger projects.
The key feature of static typing is not performance, it's reliability, readability and being able to catch various errors at compile/developing time. That's the reason why people like to use static typing and enable warnings related to static typing, not because static typing is a fancy new feature preached by a typing zealot.
GDScript's typing support is still a little half-baked, especially typed arrays, but using it is still better than not using it. I'm actually surprised that you as a developer of a reasonably large plugin didn't experience various stupid errors that could have been avoided with static typing, especially while porting the plugin from 3.x to 4.x.
Anyway, if you don't like static typing, sure that's fine, but as you said, to keep this plugin accessible by as many people as possible, at least the interface towards user code should be type friendly.

Also, nice pun.

@bitwes
Copy link
Owner

bitwes commented May 14, 2024

Types can help in some spots, but with decent naming and unit/integration tests (and occasionally some comments) you can get by just fine without them. I started GUT in Godot 2.x and I haven't felt the need to use static typing more. Sometimes user facing methods have to be more complicated due to type checking, but not often. In my personal projects I use types a little more, mostly for the autocomplete.

@bitwes bitwes merged commit 62b9bde into bitwes:main May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants