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

Add linters #85

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

Add linters #85

wants to merge 9 commits into from

Conversation

LukasMoll
Copy link

@LukasMoll LukasMoll commented Mar 26, 2023

  • Added linter configs (copied from sympy, removed exclude and per-ignore-files)
  • Applied linter code conventions

@moorepants
Copy link
Member

This is good. Thanks. You'll need to also enable a flake8 check in the CI.

@LukasMoll
Copy link
Author

LukasMoll commented Mar 26, 2023

This is good. Thanks. You'll need to also enable a flake8 check in the CI.

Thanks for the feedback. I copied and modified the runtests.yml file from the sympy project. I modified this file to only contain the instructions for flake8 for this project. I can add the others (ruff, mypy) in the future.

@LukasMoll LukasMoll changed the title Add flake8 Add linters Mar 27, 2023
@moorepants
Copy link
Member

What is the purpose of having mypy? I don't quite see how that is relevant for a set of benchmarks.

@LukasMoll
Copy link
Author

What is the purpose of having mypy? I don't quite see how that is relevant for a set of benchmarks.

I think it's nice to have to catch type related bugs and to enforce some code conventions.

Example error it's showing now:
benchmarks/solve.py:19: error: Name "expr" already defined on line 7 [no-redef]
Not sure what the purpose is of expr = None but I think I can remove it.

I understand that it can be more annoying then useful, I'll remove mypy.

@moorepants
Copy link
Member

My preference is to remove mypy.

_ = self.Case3**4
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_ = self.Case3**4
_ = self.Case3**4

@@ -239,6 +242,9 @@ def time_combined_cse(self):
cse([self.y, self.G])


S = SingletonRegistry()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this can be imported from sympy

@sylee957
Copy link
Member

sylee957 commented Mar 28, 2023

I think that __init__.py from logic-inputs should be removed. That looks like some abuse

@LukasMoll
Copy link
Author

I think that init.py from logic-inputs should be removed. That looks like some abuse

Thanks.

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.

3 participants