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

Create suggestions #30

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

murtazaverse
Copy link

Dear obonet maintainers,

I hope this message finds you well. I'm a student working on an assignment related to code review and improvement. As part of this assignment, I've made some minor enhancements to the obonet codebase that I believe could be beneficial:

  1. Added missing docstrings to some functions
  2. Improved PEP 8 compliance by splitting some long lines
  3. Renamed a few variables to follow snake_case convention
  4. Added type hints to function signatures

These changes aim to improve code readability and maintainability without altering the functionality. I'd be happy to discuss any questions or concerns you might have about these changes.

Thank you for maintaining this valuable project, and for considering my contribution.

Best regards,
Syed Muhammad Murtaza Ali Zaidi

@dhimmel
Copy link
Owner

dhimmel commented Jan 29, 2025

Thanks @murtazaverse. I am excited to see your code contributions. Would you be able to make separate pull requests for each contribution category? You can do one at a time to avoid conflicts. In my opinion, in order of importance:

  1. Added type hints to function signatures
  2. Added missing docstrings to some functions
  3. Renamed a few variables to follow snake_case convention

But order PRs according to your preference. For example, starting with variable renaming might be the easiest contribution. Note that backwards compatability is critical so let's not rename public parts of the API (but internal variables okay).

We do have some automated linting and formatted with pre-commit, so I wouldn't worry much about long lines:

- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.5.3
hooks:
- id: ruff
args:
- --fix
- id: ruff-format
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v1.11.0
hooks:

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.

None yet

2 participants