jeffwidman / bitbucket-issue-migration

A small script for migrating repo issues from Bitbucket to GitHub
GNU General Public License v3.0
314 stars 97 forks source link

Refactor, keyring, and Python 3 support #36

Closed jaraco closed 8 years ago

jaraco commented 8 years ago

Back in Februrary, I put in a great deal of effort into refactoring the code for improved maintainability. I also sought to use Python 3 without losing Python 2 support. I've since used my fork for several migrations with success. Because the project appeared abandoned, I didn't bother submitting a pull request.

Now that the project has been revived with an active maintainer (thanks @jeffwidman), I'd like to get these changes incorporated upstream. Last night, I spent a great deal of time walking through the substantial changes from @nicoddemus and others (thanks) and incorporating them into this branch.

I believe this implementation is cleaner, more robust, and of course supports Python 3. It also supports getting the password from keyring but falls back to prompting if the password isn't found. I tested it with dry run and an actual run against a test repo using the setuptools project with success.

I acknowledge that both the diff and the commit history are complicated, so I offer in lieu of requiring a thorough review to help support any issues that emerge from the acceptance of these changes. It is my intention that they are fully compatible with the existing master.

jeffwidman commented 8 years ago

Wow, thanks!

My thoughts on the code after a quick readthrough:

Anyway, those are just my quick thoughts on the actual code.

I'd like to do a couple of things here:

  1. Create a new branch on this repo, pull your changes into it, and then start squashing stuff down into logical changesets that can be merged into master. It'll be a little tedious (mostly because my git foo isn't that great) but I expect most of this work could be handled in less than a day, even factoring in all the time I'll have to spend looking up git commands.
  2. Would you be interested in being added as a collaborator on the repo?

Despite my nitpicks above, I absolutely am a fan of what you've done here, and would appreciate having a second set of eyes involved with the project if you're willing.

Just so you know--I don't expect/want this script to balloon into a huge featureset/timesink. When I took over the project, I made a shortlist of things that needed to be done (see the issues list), after which I think the script will be pretty much feature complete. At that point, I will be putting it on autopilot other than responding to the occasional PR/issue or fixing API breakage from github/bitbucket.

jaraco commented 8 years ago

I agree there are commits that needn't have happened if I'd anticipated this final outcome. Still, I come from a Mercurial philosophy that it's better to maintain history than delete it. I do see the value in having clear branches that communicate a single intention. I wouldn't mind if you want to take these ideas and incorporate them as you describe. I would even abandon my fork if the lib had the main features. I've already spent more time with this merge than I had intended, so I really can't justify making a second approach.

When it comes to helping with maintenance, I'm already overburdened with open source projects, so I wouldn't be able to do much beyond giving a second opinion on a PR or minor incidental tweaks. Still, I'd be happy to do that if it helps.

jeffwidman commented 8 years ago

Sounds good, and I understand about time constraints.

Let me see how I get on with it over the next few days. If I bang my head against the git wall too much, I may end up doing individual changes myself and just cherrypick commits out of your branch. I expect a few of them I'll ask for a code review from you to make sure I'm not missing anything.

And again, thanks! I really am excited to incorporate most of your changes.

jeffwidman commented 8 years ago

So I spent some time on this and it was just faster/simpler to write a lot of it myself, especially since I was only pulling in some of the changes.

I decided not to reorganize everything using the classes like you did--I looked at what you did, and felt like it added complexity rather than removed complexity--probably because this is a small self-contained script.

As a result, when I tried cherrypicking, I kept hitting merge conflicts. I didn't want to spend the time to fix them up, so I just referenced your commits in my commit message. On another of my projects, GitHub automatically marked these as ' committed with ', so I was expecting that to happen here.

Unfortunately, looks like I misunderstood how GitHub was identifying these, because it's not marked that way in the commit log here.... My apologies that you didn't get credit for those commits.

At this point, the only thing left from your fork that I plan to incorporate is the Keyring implementation (#39), which I'm planning to do once I get the Bitbucket authentication working for migrating private repos. So I'm going to go ahead and close this PR.

Thanks again for spending the time on it, and even though you didn't get the GitHub credit for it (again, my apologies!), your fixes and improvements are now live in the code.