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

Tests and fixes #39

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

Tests and fixes #39

wants to merge 8 commits into from

Conversation

LukeShu
Copy link
Contributor

@LukeShu LukeShu commented Sep 3, 2021

There are several unrelated things going on here, I can split them up in to separate PRs if you desire, but I've already got it all in a tidy stack of commits, and nothing here should be controversial.

  • chore: Re-generate with Go 1.17; there's a new stdlib package that was missing
  • doc fix: Remove an obsolete TODO from the README
  • Create an easy-to-extend test suite, initially seeded with the examples from the README.
  • Fix several bugs, each in a separate commit, adding testcases for each.
  • Make a stylistic change that was helpful to writing one of those fixes.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
I implemented this in daixiang0#33 but forgot to update README.md

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
For instance, it's style with nolintlint to not have a space between the
"//" and the "nolint:", yet gci puts a space there.  Let other linters
worry about whitespace, gci is for import order.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
@daixiang0
Copy link
Owner

@LukeShu please sign your commits with git commit -s --amend

@ngehrsitz
Copy link
Contributor

@daixiang0 What is the issue with the commits? To me it looks like all of them contain the Signed-off-by?

@daixiang0
Copy link
Owner

@LukeShu please read this, I guess you miss something config in GitHub :(

@LukeShu
Copy link
Contributor Author

LukeShu commented Apr 11, 2022

My apologies for the delay--at the time I was having trouble with my gpg, and then I eventually forgot about this PR.

I'll be figuring out how to update this over the next day or so. I may end up splitting it in to several PRs.

@LukeShu
Copy link
Contributor Author

LukeShu commented Apr 11, 2022

But, out of curiosity, why do you care if the commits are signed, given that you squash PRs anyway?

@daixiang0
Copy link
Owner

You can squash those into one commit and sign it.

Merge with squash does not break any signoff, it just makes git log more readable.

@LukeShu
Copy link
Contributor Author

LukeShu commented Apr 12, 2022

You can squash those

I mean figuring out how to update the contents--there are now several conflicts and so the code needs updated.

Merge with squash does not break any signoff, it just makes git log more readable.

It discards the signatures. It replaces my commits which are signed by me with a new commit--which cannot possibly be signed by me. That new commit will be signed by key 4AEE18F83AFDEB23 (GitHub (web-flow commit signing) <noreply@github.com>), regardless of whether my original commits were signed or not.

Sure, if you looked at my original commits (either by navigating to https://github.com/daixiang0/gci/pull/39 or by running git fetch origin +refs/pull/39/head) you could look at whether they were signed. But whether my commits are signed or not has absolutely no impact on the commit that ends up on master.

@daixiang0
Copy link
Owner

Thanks for your contribution, although now it has changed a lot. Feel free to update current tests if you want.

Happy to see you here!

@avorima
Copy link
Contributor

avorima commented Jun 22, 2022

Hey @LukeShu, it would be nice to see at least the fixes moving forward.

@daixiang0
Copy link
Owner

@LukeShu do you have time to update it?

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

4 participants