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

Refractor : Remove Redundant Prefixes #57

Closed
wants to merge 1 commit into from
Closed

Conversation

epicadk
Copy link

@epicadk epicadk commented Nov 28, 2020

Fixes #47

@epicadk epicadk marked this pull request as ready for review November 28, 2020 05:50
@RishabhBhatnagar
Copy link
Collaborator

Try to make sure that the commit message's subject doesn't overflow the allowed character limit.

Also, It would be better if you sign and signoff your commits.
git commit --signoff -S to be precise.

@swinslow
Copy link
Member

Thanks for this @epicadk!

Agreed with @RishabhBhatnagar, all commits need to include Signed-off-by: statements -- this is described in the CONTRIBUTING file. You can see examples in the other commits in this repo for what it should look like.

If you can resubmit with that correction in the commits, and also to address @RishabhBhatnagar's comment about the commit message subject not over-running the limit, that would be excellent. Then I'll do a closer review of the edits and hopefully we should be able to merge it in. Thank you again for your help with this!

@epicadk
Copy link
Author

epicadk commented Nov 28, 2020

Thanks for this @epicadk!

Agreed with @RishabhBhatnagar, all commits need to include Signed-off-by: statements -- this is described in the CONTRIBUTING file. You can see examples in the other commits in this repo for what it should look like.

If you can resubmit with that correction in the commits, and also to address @RishabhBhatnagar's comment about the commit message subject not over-running the limit, that would be excellent. Then I'll do a closer review of the edits and hopefully we should be able to merge it in. Thank you again for your help with this!

Would it be okay if I squashed all the commits?

@swinslow
Copy link
Member

Would it be okay if I squashed all the commits?

Sure, that's fine as long as the squashed commit has the signed-off-by line. Thanks!

@epicadk
Copy link
Author

epicadk commented Nov 30, 2020

Would it be okay if I squashed all the commits?

Sure, that's fine as long as the squashed commit has the signed-off-by line. Thanks!

I have made the requested changes. @swinslow @RishabhBhatnagar

Copy link
Collaborator

@RishabhBhatnagar RishabhBhatnagar left a comment

Choose a reason for hiding this comment

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

@epicadk, Thank You once again for your work.

@swinslow, changes LGTM. I've checked all the files and changes. We should be good to merge the changes..

@swinslow
Copy link
Member

Hi @epicadk, I'm sorry for the long delay in reviewing / responding to this!

I'm taking a look at it now. However, one thing I noticed: I see that you squashed the commits, but I'm still not seeing the Signed-off-by: line in the commit message. Can you please amend the commit message to include it with your name and email address? You can see examples from me and @RishabhBhatnagar in the other commit messages in the existing log.

After that, I'll take a look but I expect this should likely be ready to merge. Thank you for all your help!

@epicadk
Copy link
Author

epicadk commented Dec 28, 2020

Hi @epicadk, I'm sorry for the long delay in reviewing / responding to this!

I'm taking a look at it now. However, one thing I noticed: I see that you squashed the commits, but I'm still not seeing the Signed-off-by: line in the commit message. Can you please amend the commit message to include it with your name and email address? You can see examples from me and @RishabhBhatnagar in the other commit messages in the existing log.

After that, I'll take a look but I expect this should likely be ready to merge. Thank you for all your help!

Oh sorry I deleted that gpg key.

@epicadk
Copy link
Author

epicadk commented Dec 28, 2020

Sorry about that. Signed it again.

@swinslow
Copy link
Member

Hi @epicadk, sorry about the confusion. This isn't about GPG keys or using them to "sign" the commit. Rather, this is about needing to have a "Signed-off-by:" text statement in the commit message.

What this means is that, in the commit message itself, you should include a line which says the following (replacing it with your own name and email address):

Signed-off-by: Your Name <your@email.address>

This "Signed-off-by" statement follows the same process as the Linux kernel and many other open source projects. Basically, it's used to have the contributor affirm that they have the right to contribute their patch under the license specified in the file. This is the "DCO" or "Developer Certificate of Origin" process.

If you run a git log command, you'll see similar messages in the earlier commits from @RishabhBhatnagar and myself.

That addition is the change that we need to add. I've recently gotten full admin access to this repo's setup, so shortly I'll be enabling a DCO checker bot which will help to catch these issues in the future. Thank you again!

Signed-off-by: epicadk <adityakurkure@gmail.com>
@epicadk
Copy link
Author

epicadk commented Dec 29, 2020

Hi @epicadk, sorry about the confusion. This isn't about GPG keys or using them to "sign" the commit. Rather, this is about needing to have a "Signed-off-by:" text statement in the commit message.

What this means is that, in the commit message itself, you should include a line which says the following (replacing it with your own name and email address):

Signed-off-by: Your Name <your@email.address>

This "Signed-off-by" statement follows the same process as the Linux kernel and many other open source projects. Basically, it's used to have the contributor affirm that they have the right to contribute their patch under the license specified in the file. This is the "DCO" or "Developer Certificate of Origin" process.

If you run a git log command, you'll see similar messages in the earlier commits from @RishabhBhatnagar and myself.

That addition is the change that we need to add. I've recently gotten full admin access to this repo's setup, so shortly I'll be enabling a DCO checker bot which will help to catch these issues in the future. Thank you again!

Oh I understand now thanks. Made the changes.

@swinslow
Copy link
Member

@epicadk, thanks again for your help with this. I'm finally coming back to your PR here.

I've tagged a v0.1.0 release prior to merging this, so that people who are currently using spdx/tools-golang won't necessarily be totally disrupted with the significant API changes that will be made by this PR.

There are some updates in #60 that I think we're likely to merge first. After that, I'll take another pass through your PR here since I expect there will be some conflicts we may need to resolve. When I do that pass I'll also re-check whether there are other fixes or adjustments needed. But after that, we should hopefully be able to get this merged in. Thanks for your work on this and my apologies for the delay in circling back on it!

@swinslow
Copy link
Member

swinslow commented Jul 4, 2021

Hi @epicadk, thank you again for the work you put into this last year.

As the tools-golang project has continued to grow, it now has users that are making use of the existing field names. It has also evolved to add more functionality and files (e.g., those just merged for JSON parsing) that are using the existing field names.

Because of this, for the time being I think we are going to stick with the existing field names for now. So I am going to close this PR. If there is interest in the community in revisiting this later, I'll reopen the PR and implement it if feasible.

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.

[Optional Upgrade] Remove Duplicate Prefixes In The Field Names
3 participants