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

[PATCH] Removed __pycache__ #1

Merged
merged 6 commits into from
Jan 13, 2025
Merged

Conversation

Diapolo10
Copy link
Contributor

No description provided.

@Diapolo10
Copy link
Contributor Author

Since I happened to have some free time, I went and implemented some of the changes I was talking about on Reddit, and I also took a stab at the code with Ruff to lint it - fixing a couple bugs in the process, like a missing method and some erroneous method calls.

I also moved the development dependencies to pyproject.toml, if you use uv it will install them automatically when you uv sync.

I also added a py.typed file so mypy knows the code has type annotations.

@Diapolo10
Copy link
Contributor Author

Diapolo10 commented Jan 3, 2025

There were a few things I missed, apparently; first there was a bug in the tests, they were trying to access an attribute with a typo. For some additional clean-up I moved the test initialisation code to a dedicated fixture, reducing duplication.

I also noticed a slight oversight in my type annotations, and fixed that. Now everything runs quite well.

I deliberately chose not to edit pyproject.toml with my own linter rules, and I omitted committing my uv.lock file in case you wouldn't want that, but if you'd like to see an example of what I consider good linter configuration you may take a look at my newest template project which should have that covered.

@Diapolo10
Copy link
Contributor Author

If you feel I've overstepped my bounds, you're free to ignore all of this or cherry-pick the changes I implemented. That's okay, I know we all have our own philosophies and I know I can be rather stubborn sometimes, particularly with my own projects. So don't feel pressured into doing anything, you're free to do as you like!

@mtillman14 mtillman14 merged commit 06ba82f into ResearchOS:main Jan 13, 2025
@mtillman14
Copy link
Collaborator

mtillman14 commented Jan 13, 2025

Thanks for all the helpful edits! I had no issue with any of your changes. Always cool to see how a pro does it.

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