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

Refactor: Relative imports / imports without try block #200

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Root-Core
Copy link
Contributor

Cherry-picked and adopted from #152

The unit tests no longer require imports in a try block.
Also, the game fixes imports have been replaced to be relative, as this is necessary for static type checking.

@Root-Core Root-Core changed the title Refactor imports Refactor: Relative imports / imports without try block Jan 8, 2025
Copy link
Member

@R1kaB3rN R1kaB3rN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After resolving the conflict and I'll merge this ASAP

- Fixed relative imports
- Removed import handling via ImportError exception
- Fixed CI by running via module, instead of file
- Added / improved handling in __init__.py for unit tests
@Root-Core
Copy link
Contributor Author

Done, but it seems that these changes are not compatible with the new CI script.

@R1kaB3rN
Copy link
Member

Are you sure from .. import util is valid? I haven't actually tried running these fixes through Proton.

@R1kaB3rN
Copy link
Member

I'll have to test locally later.

@Root-Core
Copy link
Contributor Author

Yes, they are just relative imports. I tested them in Proton via. some games in Steam. This also enables Pyright and transforms the code base from a bunch of independent files that somehow references each other to an actual project.

I'm not sure how it even references protonfixes in the current implementation.
I guess the python call has the module injected somehow? With the relative imports, there aren't any confusions.

@R1kaB3rN
Copy link
Member

Yes, they are just relative imports. I tested them in Proton via. some games in Steam. This also enables Pyright and transforms the code base from a bunch of independent files that somehow references each other to an actual project.

I'm not sure how it even references protonfixes in the current implementation. I guess the python call has the module injected somehow? With the relative imports, there aren't any confusions.

Well, I can reproduce the error. Change the directory to gamefixes-steam, invoke python, then try to import with from .. import util. This will fail.

@R1kaB3rN
Copy link
Member

I guess another way is to use umu-launcher and run all the fixes through it which I have been avoiding. Because of course, it'll take a while though even when running it concurrently.

@Root-Core
Copy link
Contributor Author

Root-Core commented Jan 10, 2025

Well, I can reproduce the error. Change the directory to gamefixes-steam, invoke python, then try to import with from .. import util. This will fail.

This is expected behaviour in Python, but imports can get ugly pretty fast there anyways.
That's why we launch the unit test from outside of the module.. or we need to fixup the imports via an try block.

I guess another way is to use umu-launcher and run all the fixes through it which I have been avoiding. Because of course, it'll take a while though even when running it concurrently.

I don't think this is necessary, as pyright is a well known and maintained solution.
I'm not a huge fan of spinning an home-brewed solution for something that's already solved.

Nonetheless, you would only need to run it once over the full set of fixes. The github workflow should give you a list of changed files, as far as I know. This could be used for incremental checks.

EDIT:
We could, maybe, convince PyRight to find the protonfixes module somehow via it's configuration.
A vector that I found might be executionEnvironments[], more specific extraPaths[]. I have not tested it, nor do I think that's a better solution.. but it might enable PyRight to resolve the module.

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.

2 participants