Skip to content

Conversation

@harryswift01
Copy link
Contributor

@harryswift01 harryswift01 commented Mar 7, 2025

Summary

This PR implements a CI pipeline and pre-commit hooks to maintain high coding standards when pushing to the repository.

Changes

Pre-commit hooks :

  • Introduced a .pre-commit-config.yaml to enforce standards when commiting to this repository.
  • Made use of Black, pre-commit hooks, flake8 and isort.
  • Was originally going to use pylint as well but this would introduce more manual changes, could be something to look into down the line.
  • Applied 'pre-commit run --all-files', this automatically formatted a majority of the code, some manual intervention was needed.

CI Pipeline :

Pull Request Template :

  • Standardised the pull request template to streamline PR's moving forward and to add more detail and understanding of the changes and it's impact on the codebase.

Numpy naming convention:

Impact

  • Reformatted the entire codebase to align with proper Python standards.
  • Enforced these coding standards on every push to the repository.
  • Improved Developer Workflow, reduces manual formatting and linting, allowing developers to focus on writing code.
  • Enhanced Code Consistency, ensures a uniform coding style across the repository, making it easier for new contributors.
  • Increased CI Reliability, automating tests and enforcing standards at the CI level helps catch issues early.
  • Better Future Maintainability, simplifies refactoring and code reviews, reducing technical debt.
  • Improved Scalability, helps maintain consistency as the project grows, making collaboration more efficient.

- Introduced a .pre-commi-config.yaml to enforce standards when commiting to this repository.
 - Includes Black, pre-commit-hooks, flake8 and isort.
- Applied 'pre-commit run --all-files', this automatically formatted a majority of the code, some manual intervention was needed.
- Addressed issue #41 with ensuring any instances of numpy is refered to as np. and not .nmp
- Updated CI.yaml to allow Github Actions to run:
 - tests
 - docs
 - pre-commit
- Moved the tests directory to the root of the repository, mentioned in issue #40
Copy link
Member

@jimboid jimboid left a comment

Choose a reason for hiding this comment

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

A couple of things to check.

_version.py - do we actually need this since we are now moving away from setuptools approach in setup.py. I think this might have been some code form a cookie cutter.

There are a few stray comments using "#" with nothing there after it so these can be deleted.

There are a number of missing docstrings on methods, can you record somewhere that we need to get Sarah to search for """ """ and add the missing docstrings, if we need to seek help from others then we should.

Can you also make note of there are many comments at the end of methods that simply say end of method. This is not a python pattern of coding, these should be removed in a future PR.

The only thing I would really like to see altered before merge is the _version.py removed if safe. If it is not safe then please comment back and I will approve for merge.

- Removed CodeEntropy/_version.py as this is no longer needed
- Remove stray comment markers in CodeEntropy/PoseidonHelper.py
- Added in missing docstring for CodeEntropy/poseidon/analysis/helper.py
- Removed brackets in comments for tests/test_EntropyFunctions/test_frequency_calculation.py
@harryswift01 harryswift01 marked this pull request as ready for review March 7, 2025 17:43
Copy link
Member

@jimboid jimboid left a comment

Choose a reason for hiding this comment

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

All good to merge. Thanks a lot Harry!

@harryswift01 harryswift01 merged commit 51109c1 into main Mar 7, 2025
@harryswift01 harryswift01 deleted the 39-implement-ci-pipeline-pre-commit-hooks branch March 7, 2025 18:07
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.

Consistent Naming of numpy Across the Codebase Implement CI Pipeline & Pre-commit Hooks

3 participants