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

TODO: Extend lint rules for Pylint #50

Open
R1kaB3rN opened this issue Mar 29, 2024 · 9 comments
Open

TODO: Extend lint rules for Pylint #50

R1kaB3rN opened this issue Mar 29, 2024 · 9 comments
Assignees

Comments

@R1kaB3rN
Copy link
Member

R1kaB3rN commented Mar 29, 2024

Related to #48 (comment)

For quality assurance and to prevent bugs such as ac9ddf2, more lint rules should be extended and applied to the Protonfix/gamefix modules.

@R1kaB3rN R1kaB3rN self-assigned this Mar 29, 2024
@Root-Core
Copy link
Contributor

Root-Core commented Mar 29, 2024

If I don't misunderstand you, this is already the case with commit c956ac9, the , adds the files on the root level.

Well, we could just enforce it on all *.py files - without the "static" array.

@R1kaB3rN R1kaB3rN changed the title TODO: Enforce *.py files via the linter TODO: Extend lint rules for Pylint Mar 29, 2024
@R1kaB3rN
Copy link
Member Author

If I don't misunderstand you, this is already the case with commit c956ac9, the , adds the files on the root level.

Sorry, my description was sloppy/inaccurate there. Thanks for the correction.

Well, we could just enforce it on all *.py files - without the "static" array.

Yeah and probably define some files to exclude such as __init__.py for the gamefixes in pyproject.toml

@Root-Core
Copy link
Contributor

No problem. I moved the linked comment here for better discussion. I also expanded it a bit.


We should enforce this via linter:

  • function types
  • single quotes
  • format strings
  • Docstrings for each function (C0116)
  • Docstrings indentation on multiple lines
  • no blank lines after function Docstrings
  • two blank lines between functions
  • a reasonable line length (?)

We want to:

  • Check if there are unused functions
  • Check that GDrive is actually working
  • Split large modules like util.py into smaller ones
  • Enforce linter on all *.py files
  • Handle the complete linter configuration in the pyproject.toml file
  • Remove #pylint: disable=C0103 from gamefixes
  • Have as few pylint exceptions as possible

@R1kaB3rN
Copy link
Member Author

R1kaB3rN commented Apr 15, 2024

I think enforcing rules for Docstrings will require additional tools than pylint. Personally, I'd like to just have one tool for the sake of simplicity.

In addition, there should probably as much tests as possible. In particular for critical files such as fix.py and util.py

@R1kaB3rN
Copy link
Member Author

R1kaB3rN commented Sep 2, 2024

I have a PR open that's addresses some of these issues.

As for these items:

  • Check that GDrive is actually working
  • Split large modules like util.py into smaller ones

I've already confirmed that the GDrive functionality does not work, but I believe it would be best to address them in a separate PRs where we can consider removing some functionality. In fact, there shouldn't be a need to support Google drive anyway as users can upload files to Github.

@Root-Core
Copy link
Contributor

Root-Core commented Sep 3, 2024

  • I think the GDrive isn't really useful and likely to break. We might just remove it.
  • Splitting the modules is a good thing. I'm working on a bit of cleanup of the fixes.
  • I thought we could create something like gamefixes-common and add some basic fixes - like the disabling the media converter and sym-link to them. A lot of game fixes are one-liner and could be replaced. Not sure if that's a good idea though.
  • I will add a test that util.protonfixes exist.. to prevent something like wmp9_x86_64

EDIT:
I had a look at your PR and it includes basically my changes.. like enforced types and removal of pylint-exceptions.. I will wait for it to be merged, before I continue doing bigger changes.

@R1kaB3rN
Copy link
Member Author

R1kaB3rN commented Sep 4, 2024

Just merged the PR.

Yeah, I support removing the GDrive functionality. Same with splitting the modules, which will be a large change. To that end, to reduce the likelihood of potential symbol errors, I think it would be worth adding a tool like pyright in the CI.

@Root-Core
Copy link
Contributor

We should also implement a README with some hints and explanations. For example, how to run the checks locally, and what a good fix should look like / how the umu-id works for non-steam games (or links to the corresponding repo).

@R1kaB3rN
Copy link
Member Author

R1kaB3rN commented Sep 4, 2024

We should also implement a README with some hints and explanations. For example, how to run the checks locally, and what a good fix should look like / how the umu-id works for non-steam games (or links to the corresponding repo).

Agreed, especially for running/testing checks locally which isn't as simple anymore. Since the merge of the build system, some binaries were removed, so users can't simply just replace a Proton build's entire protonfixes directory anymore for some cases. For example, protonfixes.util.get_resolution expects xrandr to be within the project directory for it to work.

Also, for users who want to add a non-Python library, they will be expected to compile the project within the Steam Runtime, copy the dist/protonfixes directory to a GE-Proton directory generated from make install, and move, if there are any, the *.so file files to the Steam Runtime's LD_LIBRARY_PATH. If that all works, then finally update the GE-Proton/UMU-Proton's Makefile to copy those files at build time. To improve the developer/tester UX, some of these steps should ideally be automated with a simple script.

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

No branches or pull requests

2 participants