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

Feature/provider selection #125

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

croesnick
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

Currently, only git repositories are supported.

What is the new behavior?

But in my uses cases, I additionally need svn and also git-svn (as a proxy for svn repos). This PR adds both. It moreover disables all git-related functions (gitlab export and alike) when the provider is not git.

Does this PR introduce a breaking change?

  • Yes
  • No
  • Column sha in CSV has been renamed to revision. I though about deprecating the sha column, but then then new revision column would needed to go to the end of the csv, which IMHO is not ideal. Rationale for just doing the renaming has been that the upgrade path after this change is very easy: simply rename sha to revision in the csv and we are good to go. In case this is still an inconvenience, we could handle the revision being stored either in the revision or sha property when loading the csv and just not telling the user about it.
  • Configuration option code-review.gitDirectory has been moved to code-review.vcs.git.directory

Other information

Copy link
Owner

@d-koppenhagen d-koppenhagen left a comment

Choose a reason for hiding this comment

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

Good job @croesnick! There is one big thing with the change from shato revision:
I like the change but will will introduce a breaking change.
So existing consumers which already have sha in the comments file have to migrate.
My suggestion:

  • provide a migration task, that will convert the file-header into the new format
  • once we will merge this features, I will introduce a breaking change in the version so that consumers are aware of the changes (just in case the migration task fails)

Oh and as I merges #124 , a rebase in needed

croesnick and others added 8 commits February 19, 2022 17:10
@d-koppenhagen
Copy link
Owner

@croesnick I recently merged some other features/fixes, so a rebase is needed.
To not beak the DevEx for current consumers, the adoption from the sha to revision CSV Header name should be done automatically, when reading existing comments.

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.

2 participants