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

Consistent code style #62

Open
MarJMue opened this issue Jun 29, 2022 · 9 comments
Open

Consistent code style #62

MarJMue opened this issue Jun 29, 2022 · 9 comments

Comments

@MarJMue
Copy link
Collaborator

MarJMue commented Jun 29, 2022

Run all subfiles through a linter and an formatter, to get a consistent style.

Part of the code is already formatted with black, so it should be used everywhere.

@domna
Copy link
Member

domna commented Jun 30, 2022

Should we maybe also build an Action to check linting?

@MarJMue
Copy link
Collaborator Author

MarJMue commented Jun 30, 2022

👍

@domna
Copy link
Member

domna commented Jul 26, 2022

Formatted according to black and added actions for black linting and sphinx-lint

@domna domna closed this as completed Jul 26, 2022
@domna
Copy link
Member

domna commented Jul 26, 2022

@MarJMue Shall we enforce pylint as pre-commit hook and GitHub action?
Sometimes I find pylint a little over-restrictive but we can deactivate it with # pylint: disable=... to show that we deliberately did something else.

@domna domna reopened this Jul 26, 2022
@MarJMue
Copy link
Collaborator Author

MarJMue commented Jul 26, 2022

I am not really sure. I think pylint has a lot going for it, but as you said there are a more then a few false positives and I do not like to add even more comment based command in the source code, e.g. before every single character variable taken from literature.

Maybe if we disable a few selected warnings...

@domna
Copy link
Member

domna commented Jul 26, 2022

Yes, I think we should disable some warnings globally and add the short names to the --good-names options when running pylint to accept them. Then it should mainly work w/o any inline comments to the code.

@domna
Copy link
Member

domna commented Jul 27, 2022

I added a pylintrc and we now support the regex pattern [a-zA-Z][a-zA-Z0-9_]{0,30}$ for names of variables, arguments and attributes throughout the whole project. Can be used with --rcfile=.pylintrc when invoking pylint, but vscode picks it up by default.

@MarJMue
Copy link
Collaborator Author

MarJMue commented Jul 28, 2022

Looks really good. But i will still try to remove any unnecessary uses of single character variables, while writing more documentation.

@domna
Copy link
Member

domna commented Jul 28, 2022

Yes, I think it's generally good to try to still follow the general naming style. But there really are plenty of variable names which are just naturally written in another style in physics which I would not want to break (e.g. Eg).

I still have to refine the regex pattern as well, e.g. we should allow underscores in the beginning for attributes to mark them as "private". But I at some point I will sit down and go through the pylinting issues and try to fix them and then we can introduce an action for checking it.

@MarJMue MarJMue modified the milestone: 1.0.0 Sep 13, 2022
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