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

SCons: Add unobtrusive type hints in SCons files #93058

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Jun 11, 2024

This PR adds scons_hints.py to the repo, which grants intellisense logic to all SCons files. Like #86213, this doesn't change any of the script files themselves outside of the include statement; however, the actual line is even more compact & setup to play nice with our new ruff formatter. Now all SCons files will have this header:

#!/usr/bin/env python
from misc.utility.scons_hints import *

No space after the shebang is deliberate, as this new line is effectively metadata. The comments at the end are to disable a warning for a star import (not added to pyproject.toml, as this should be warned about for all other cases) & to not account for this line when performing isort (EDIT: Removed comments; logic handled entirely by pyproject.toml). Opted to put the hint file in misc/utility to avoid cluttering the root, and the added verbosity helps the line feel more "no-touchy".

The only other changes were making isort automatically put the metadata import on top & adjusting the per-file-ignores for SCons files:

  • F821 (Undefined name) was removed, as now all the names are properly defined.
  • F403 (Import star) was added, as this setup relies on the import star syntax.
  • F405 (Import star variable) was added, same as above.

@Repiteo Repiteo added this to the 4.x milestone Jun 11, 2024
@Repiteo Repiteo requested review from a team as code owners June 11, 2024 20:40
@Repiteo Repiteo removed the request for review from a team June 11, 2024 20:40
@Repiteo Repiteo removed request for a team June 11, 2024 20:40
@Repiteo Repiteo force-pushed the scons/scons_hints branch 3 times, most recently from 9ae0139 to 6dd66cd Compare June 15, 2024 15:58
@Repiteo
Copy link
Contributor Author

Repiteo commented Jun 15, 2024

Made a slight adjustment to no longer need the # noqa: F403 # isort: skip comment. Turns out that tool.ruff.lint.isort has options to define import order, so I made a dedicated metadata section which includes misc.scripts.scons_hints exclusively. Now tools don't have to worry about out-of-order sorts from automated IDE actions, as isort will ensure the metadata section is always on top. Additionally, F403 was added to the per-file ignores alongside F405, as it's the only instance of star importing in the entire repo & is isolated to SCons as-is.

@Repiteo Repiteo force-pushed the scons/scons_hints branch from 6dd66cd to 1576370 Compare June 23, 2024 13:24
@Repiteo Repiteo force-pushed the scons/scons_hints branch 2 times, most recently from 66d1a87 to 70ca8fe Compare July 13, 2024 13:42
@Repiteo Repiteo modified the milestones: 4.x, 4.3 Jul 13, 2024
@Repiteo
Copy link
Contributor Author

Repiteo commented Jul 13, 2024

With the belated due-date of 4.3 & largely indirect nature of this implementation, this felt like a safe milestone adjustment. Feel free to revert if this feels out-of-scope.

@akien-mga akien-mga modified the milestones: 4.3, 4.x Jul 22, 2024
@Repiteo Repiteo modified the milestones: 4.x, 4.4 Sep 25, 2024
@Repiteo Repiteo requested a review from akien-mga September 25, 2024 14:37
@akien-mga
Copy link
Member

Looks good to me.

I'd just like to confirm that this stays fully optional, and not something we plan to enforce with more CI checks in the future. Third-party modules or platforms might have their own SCsub files which may or may not be updated to match the changes in this PR, and we don't want to break them.

@Repiteo
Copy link
Contributor Author

Repiteo commented Sep 25, 2024

It'd be 100% optional. I'll still make an effort to ensure every SCons file within the main repo uses it, but that should all be manageable manually.

@Repiteo
Copy link
Contributor Author

Repiteo commented Sep 25, 2024

The best point of comparison I can give is it's equivalent to the #!/usr/bin/env python shebang on the SCons files. While using a shebang in a non-executable context is technically invalid, it's to the end of giving the files proper syntax/hinting out of the gate; that is, it only affects IDE contexts. Likewise, we don't have any hard enforcement for adding #!/usr/bin/env python to the top of SCons files (though it's good practice & everyone does it anyway).

@akien-mga akien-mga merged commit a90da7e into godotengine:master Sep 26, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

2 participants