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

Discuss possible code checks to be used in meshpy #108

Closed
7 of 8 tasks
isteinbrecher opened this issue Oct 2, 2024 · 9 comments
Closed
7 of 8 tasks

Discuss possible code checks to be used in meshpy #108

isteinbrecher opened this issue Oct 2, 2024 · 9 comments
Assignees

Comments

@isteinbrecher
Copy link
Collaborator

isteinbrecher commented Oct 2, 2024

This issue shall serve as a spot to discuss possible code checks and pre-commit hooks to be used within MeshPy. Some possible choices can be seen here https://github.com/davidrudlstorfer/pyskel/blob/main/.pre-commit-config.yaml (thanks to @davidrudlstorfer).

Possbile tools:

  • pylint currently we have 8.93/10 for meshpy/ and 8.55/10 for tests/
  • Ruff
  • Code formatter for markdown could not be found
  • Code formatter for c++
  • Code formatter for Cython. Does such a tool exist? Do we need this as the Cython code likely will not change on a regular basis. Deemed not worth the effort
  • Typo check (see Introduce typo hook #163)

Other code format to enforce:

@isteinbrecher isteinbrecher changed the title Discuss possible code checks to be used Discuss possible code checks to be used in meshpy Oct 2, 2024
@davidrudlstorfer
Copy link
Collaborator

To note it for later discussion; we could also utilize ruff (https://github.com/astral-sh/ruff) which replaces flake8, isort and black in one tool. This tool is currently spreading fast

@isteinbrecher
Copy link
Collaborator Author

I think with #153 we got a very long way towards completing this issue. Are there still some things missing? Two I can think of are the required docstrings and prohibiting relative imports (I added them to the description).

@davidrudlstorfer
Copy link
Collaborator

#153 includes a first suite of pre-commit hooks and also utilizes it within the code check pipeline

Further points to complete code checks are:

  • Enforce absolute imports, i.e., meshpy.rotation.blablub rather than ..rotation.blablub
  • Enforce the docstrings, currently we have 96% coverage => add remaining docstrings and set 100% coverage
  • Enforce the docstring style, i.e. args to be consistent across the code base
  • Type hinting - I think we briefly talked about this and you had some points against that?
  • Enforce that each source code file contains the license header

@davidrudlstorfer
Copy link
Collaborator

@davidrudlstorfer
Copy link
Collaborator

Regarding the xml formatting and syntax check on vtu/vtk files:

Yesterday I wasted a lot of time to get this running and I was not able to get it to work at all. I was not able to add differing file endings to the xml hooks.

We probably need our own special hook for that. Maybe we can just take the contents of the vtu/vtk files and hand it over to the xml hooks.

@isteinbrecher
Copy link
Collaborator Author

isteinbrecher commented Dec 17, 2024

That was just a suggestion (the xml formatting for vtu files), we don't have to do that if it is complicated. The relevant files are almost never created by hand so I am fine with just sticking with the default format there (now that I think of it, I think in 4C we also don't format those files - at least I never had any issues checking in the automatically created ones).

@isteinbrecher
Copy link
Collaborator Author

@davidrudlstorfer is it possible to automatically format standard comments (after a #) similar to docstrings?

@davidrudlstorfer
Copy link
Collaborator

davidrudlstorfer commented Jan 9, 2025

I recently looked a bit into auto formatting comments but was not able to find any specific pre-commit hooks which could do that.

The only related thing I found was astral-sh/ruff#7414

So once ruff supports it we can simply turn it on in out pre-commit hook

In my opinion we can soon close this issue as all possible code checks are in place after #167 is merged. I would propose to add the TODO for the relative imports in the issue #159

@isteinbrecher
Copy link
Collaborator Author

I agree, let's close this issue as the "initial" pre-commit wave is completed and open individual issues for new hooks in the future. I will move the point regarding the relative imports to #159

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