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

refactor(provider): split provider code and related tests into individual files for maintainability #830

Merged
merged 39 commits into from
Sep 9, 2023

Conversation

davidlday
Copy link
Contributor

closes #812

Description

Refactor provider code and related tests into individual files for maintainability

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

N/A - refactor

Steps to Test This Pull Request

N/A - refactor

Additional context

N/A - refactor

@davidlday davidlday changed the title refactor: split provider code and related tests into individual files for maintainability refactor(provider): split provider code and related tests into individual files for maintainability Aug 28, 2023
@codecov
Copy link

codecov bot commented Aug 28, 2023

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

Hi @davidlday , thanks for taking the time for this refactoring. I've not yet had the chance to finish the review. But left a few comments on what's need to be fixed before we merge it. The ones in test cases can be applied to all the newly created test files

commitizen/providers/__init__.py Outdated Show resolved Hide resolved
pyproject.toml Outdated
pep621 = "commitizen.providers:Pep621Provider"
poetry = "commitizen.providers:PoetryProvider"
scm = "commitizen.providers:ScmProvider"
cargo = "commitizen.providers.cargo_provider:CargoProvider"
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a commitizen/providers/__init__.py to avoid this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally added them all to __init__.py, but ruff removed them when I ran scripts/format due to F401 (unused import). I can override in __init__.py, if that makes sense for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Lee-W - went ahead and added # noqa: F401 to the import lines in __init__.py and reverted the changes in pyproject.toml. Let me know if you want this handled differently.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

tests/providers/test_base_provider.py Outdated Show resolved Hide resolved
tests/providers/test_composer_provider.py Outdated Show resolved Hide resolved
@Lee-W
Copy link
Member

Lee-W commented Sep 4, 2023

might need your help to rebase from the master branch

@davidlday
Copy link
Contributor Author

might need your help to rebase from the master branch

Will get to it in the next hour.

@woile
Copy link
Member

woile commented Sep 5, 2023

LGTM as well. Thanks for this change!

@Lee-W
Copy link
Member

Lee-W commented Sep 5, 2023

Hi @davidlday I think we're almost good to go. Could you please rebase the branch again? Thanks!

@davidlday
Copy link
Contributor Author

@Lee-W - rebased

@Lee-W
Copy link
Member

Lee-W commented Sep 6, 2023

Hi @davidlday , sorry for being annoying 🫠 May I know how you rebase the branch? I notice there are a lot of duplicate commits. Or I could also just squash this PR into 1 commit

@davidlday
Copy link
Contributor Author

Hi @davidlday , sorry for being annoying 🫠 May I know how you rebase the branch? I notice there are a lot of duplicate commits. Or I could also just squash this PR into 1 commit

Not annoying at all. I used git rebase -i master, but I accidentally included the duplicates instead of squashing or dropping them and cherry-picking the right ones. It's a mistake, not intentionally. I can rebase again and squash into a single commit if that'll help. I think I mentioned in my last PR, I'm very rusty on rebasing since I rarely use it. Let me know what you want me to do.

@davidlday
Copy link
Contributor Author

@Lee-W @woile - I realize now that I've been rebasing incorrectly. At this point, I'm more than willing to close this PR & and start a new one on a fresh branch for a cleaner commit history. I should have time to do so this evening if that works for you two. Changes are easy enough to recreate. My apologies!

@Lee-W
Copy link
Member

Lee-W commented Sep 7, 2023

@davidlday I was thinking of squashing it into a commit, but I haven't had the chance to do so yet. 🫠 However, if you'd like to make a new one, I'm completely okay with it. 👍 You don't need to apologize, you're doing an excellent job, and I really appreciate it!

@davidlday
Copy link
Contributor Author

@Lee-W - thank you for the kudos. If you're okay with squashing to a single commit, I'd prefer that, so we can move on without risk of me making a bigger mess. 😄 I appreciate your patience, and I appreciate the wonderful work you and @woile and all the other contributors have done here!

@Lee-W Lee-W merged commit 92903cf into commitizen-tools:master Sep 9, 2023
18 checks passed
@davidlday davidlday deleted the chore/provider-refactor branch September 9, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Refactor] Restructure Provider code and tests
3 participants