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

Decide whether to keep the new Git classes or go back to GitPython #23

Closed
uliska opened this issue Mar 4, 2020 · 3 comments
Closed

Comments

@uliska
Copy link
Contributor

uliska commented Mar 4, 2020

PR #12 introduced a number of Git classes - essentially a small subset of Git functionality, just what is needed for the plugin, and dropped the dependency on GitPython.

It should be thoroughly considered whether this should be kept or if we should revert to GitPython. The main factors to consider are:

  • small amount of code tailored to our needs
    and
    no dependency
    vs
  • mature external library, relaying the responsibility to that domain to someone else
@timvink
Copy link
Owner

timvink commented Mar 4, 2020

I'm impressed you're open to considering this after putting in so much work.

The pro's are clear to me, the only counterargument I had was stability. Now that we (or at least me) have learned that GitPython uses git blame --porcelain, and 6f5822c implements that, I see no reason to keep using GitPython (except in the unit tests).

The new git classes are a great addition, make the package easier to extend & more lightweight, without compromising on stability.

So I consider this issue closed.

@timvink timvink closed this as completed Mar 4, 2020
@uliska
Copy link
Contributor Author

uliska commented Mar 4, 2020

It was pure rationality, I'd say. I made these changes without real prior discussion, so I can't request they are kept. Of course I want it to be used, but I also know I can't consider the issue unbiased, simply given the amount of work I put into it.

But I agree that we should thoroughly test this. When switching to --porcelain I stumbled over a strange case: When the first instance of a commit on a page is skipped as an empty line, the second instance will not have the commit metadata available, which caused a crash down the road.
The remedy was simple, but I know that I can't be sure there are no further corner cases that might break things.

@timvink
Copy link
Owner

timvink commented Mar 5, 2020

When the first instance of a commit on a page is skipped as an empty line, the second instance will not have the commit metadata available, which caused a crash down the road.

I'll make sure to add a unit test for that, and other patterns I can think of.

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

No branches or pull requests

2 participants