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: ruff SConstruct/SCsub fixes #92264

Closed

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented May 22, 2024

Now that ruff is properly merged as our linter/formatter of choice, it's now appropriate to fix the previously ignored warnings for SConstruct/SCsub files: E402 & F821. As indicated by this PR changing 177 files, this wasn't exactly suitable for that initial PR. Despite the amount of files changed, the majority of changes were assigning some variation of this to the top of every SCsub:

from typing import TYPE_CHECKING

from SCons.Script.SConscript import SConsEnvironment

if TYPE_CHECKING:
    env = SConsEnvironment()
    Import = SConsEnvironment.Import

Remaining changes were ensuring imports at the top level (where applicable) & changing most "native" SCons functions into explicit class calls via env to make them typed. With these changes in place, it's now feasible for a followup PR to make the SConstruct/SCsub files support mypy checks (currently only applied to .py files), as the major typing ambiguities will have been dealt with.

@Repiteo Repiteo added this to the 4.x milestone May 22, 2024
@Repiteo Repiteo requested review from a team as code owners May 22, 2024 18:31
@Repiteo Repiteo removed request for a team May 22, 2024 18:37
@fire
Copy link
Member

fire commented May 22, 2024

May have to update the guides at https://docs.godotengine.org/en/stable/contributing/development/core_and_modules/custom_modules_in_cpp.html

@akien-mga
Copy link
Member

I'm not sure solving these warnings actually improves the quality of our code. It's a lot of boilerplate to workaround the design of SCons.

@adamscott
Copy link
Member

adamscott commented Jun 11, 2024

I suggest a lighter way, but as powerful.

We can import variables in python. How about we do this?

# /typed_scons.py
from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from SCons.Script.SConscript import SConsEnvironment
    env = SConsEnvironment()
    Import = SConsEnvironment.Import

Then, in any SCsub file, we can do:

# /platforms/web/SCsub
from typed_scons import *

# ... rest of the SCsub file

image

@Repiteo
Copy link
Contributor Author

Repiteo commented Jun 11, 2024

That's similar to another PR I've setup previously, #86213, but ruff doesn't love the syntax:
24-06-11 14-15-19 Code
…Though, now that I think about it, these star-import warnings could always be excluded for SConstruct/SCsub files specifically. It'd mean suppressing warnings still, but this time for good reason as they'll be setup with intent. Will play around with this later, albeit most likely in another PR as this one is already pretty shaken up beyond the hints.

@adamscott
Copy link
Member

Yeah, I kinda forgot about installing ruff on VSCode. Sorry!

@Repiteo
Copy link
Contributor Author

Repiteo commented Jun 11, 2024

Superseded by #93058

@Repiteo Repiteo closed this Jun 11, 2024
@Repiteo Repiteo removed this from the 4.x milestone Jun 11, 2024
@Repiteo Repiteo deleted the scons/ruff-SConstruct-SCsub-fixes branch June 11, 2024 20:42
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.

4 participants