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

Basic Linting of the repo #15

Merged
merged 9 commits into from
Mar 18, 2024
Merged

Basic Linting of the repo #15

merged 9 commits into from
Mar 18, 2024

Conversation

surbhigoel77
Copy link
Collaborator

@surbhigoel77 surbhigoel77 commented Feb 22, 2024

Fixes #4

The PR is to lint the entire repo to improve the quality of the code. Things formatted are in the form of:

  • Docstrings
  • Blank Lines
  • White spaces
  • Extra long lines

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Documentation

Key checklist

  • Make sure you've run the pre-commit checks: $ pre-commit run -a
  • All tests pass: $ poetry run pytest

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@surbhigoel77 surbhigoel77 self-assigned this Feb 22, 2024
@surbhigoel77 surbhigoel77 added enhancement New feature or request Python-repo Part of the python NN repo labels Feb 22, 2024
@surbhigoel77 surbhigoel77 marked this pull request as draft February 22, 2024 13:59
@jatkinson1000 jatkinson1000 linked an issue Feb 26, 2024 that may be closed by this pull request
@surbhigoel77 surbhigoel77 mentioned this pull request Feb 26, 2024
@surbhigoel77 surbhigoel77 marked this pull request as ready for review March 4, 2024 13:19
@surbhigoel77
Copy link
Collaborator Author

Using ruff to lint the existing code. One of the most highlighted formatting error is missing docstrings for defined classes and functions.

@surbhigoel77
Copy link
Collaborator Author

Doctsrings have been added to all the classes and existing functions. Restructuring the code may require updating these docstrings accordingly in future.

@surbhigoel77 surbhigoel77 requested a review from yqsun91 March 4, 2024 17:29
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

All seem sensible so far - mostly formatting changes.

The docstrings are somewhat hybrid at the moment, but we should make sure they conform to the numpy style: https://numpydoc.readthedocs.io/en/latest/format.html
Main one I can spot at the moment is args should be Parameters.

pyproject.toml Show resolved Hide resolved
@mondus
Copy link

mondus commented Mar 11, 2024

To merge after addition of CI and once Jacks final comments is addressed (internally). Open a new issue on Docstrings so that these can be updated with a bit more detail after #5.

@surbhigoel77
Copy link
Collaborator Author

All seem sensible so far - mostly formatting changes.

The docstrings are somewhat hybrid at the moment, but we should make sure they conform to the numpy style: https://numpydoc.readthedocs.io/en/latest/format.html Main one I can spot at the moment is args should be Parameters.

Not changing this at the moment as the ML code is being refactored. Once that is done, I will update the docstrings acc to the numpy format.

Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

I realise that this is failing CI, but that's sort of the point of this PR.
I suggest merging now and then working to come towards compliance with the rest of the code, perhaps in #5 and #6 which will require a rebase on top of main once this is merged.

@jatkinson1000 jatkinson1000 merged commit ad27b8b into main Mar 18, 2024
1 check failed
@jatkinson1000 jatkinson1000 deleted the linting branch March 18, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Python-repo Part of the python NN repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tidy the code (basic)
3 participants