energywebfoundation / iam-client-lib

TypeScript library to be used within decentralised applications for authentication and authorisation using DIDs (Decentralised Identifiers) and VCs (Verifiable Credentials)
GNU General Public License v3.0
19 stars 9 forks source link

#icl 317 #581

Closed whitneypurdum closed 2 years ago

whitneypurdum commented 2 years ago

Description

Contributor checklist

whitneypurdum commented 2 years ago

I don’t understand when you have to squash? Is it bad practice to have multiple commits? If you commit early and often you will always have multiple commits, and if you have to apply formatting etc. Is the goal to have as few commits as possible? If you have to address Pr comments, you will also have additional commits, there is no way to avoid it.

whitneypurdum commented 2 years ago

Thanks, looks good. Maybe this PR could use some squashing as well 😄

Squashed!

jrhender commented 2 years ago

My perspective is that the priority should be having the best commit history in the "main" branches (develop and master at this point). The primary reason for this is that we are currently basing our changelog (https://github.com/energywebfoundation/iam-client-lib/blob/develop/docs/CHANGELOG.md) on the commits. So looking at the commits in this PR, we had commits like fix: apply prettier formatting and fix: do not make test async. I think it's very reasonable to have them in the PR as it is being worked on, but I think they don't add much value to the commit history in develop and to the changelog as they aren't really relevant changes to the overall history, but are instead more edits to improve the first commit as it were. (at least that's how I see). By the way, for style commits we have a style prefix that could be used (if the commit was to be included) https://github.com/energywebfoundation/shared-configs/blob/cfd53391204ebc92f3414b797173b2a16cc2220c/packages/semantic-release-config/index.js#L7

To be clear, I'm not saying every PR should be a single commit. Having granular commit can be really helpful to know why a particular change was made and makes it easier to undo a change.

I suppose, in summary, the guiding questions for me are "does it make sense for this commit to be on our public changelog?" and "does is make sense for this commit to be in the develop history?"

@whitneypurdum what's your take on the above?

ewf-devops commented 2 years ago

:tada: This PR is included in version 6.0.0-alpha.17 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

ewf-devops commented 2 years ago

:tada: This PR is included in version 6.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: