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

feat(adapter): refactor to engine, default to conventional commit types #30

Merged
merged 1 commit into from
Aug 14, 2016

Conversation

AndersDJohnson
Copy link
Member

@AndersDJohnson AndersDJohnson commented Aug 9, 2016

Closes #29.

Changes due to relying on conventional-commit-types include addition of a revert type and some minor formatting changes in the prompter choices for types due to newlines & indentation being lost in type descriptions - I felt those were too specific to this use case to include in the types module.

If merged, next step is to pull engine.js into separate module per @LinusU at #29 (comment).

I can give ownership of conventional-commit-types repo to commitizen and all the same npm owners as this module has.

@AndersDJohnson AndersDJohnson force-pushed the engine-with-commit-types branch from beeb191 to b7a6ca0 Compare August 9, 2016 23:45
@AndersDJohnson AndersDJohnson changed the title feat: refactor to engine, default to conventional commit types feat(adapter): refactor to engine, default to conventional commit types Aug 9, 2016

options = options || {};

var types = objectAssign({}, options.types || conventionalCommitTypes.types);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to make this a required property, so that the engine doesn't need to depend on conventional-commit-types? I feel that that should be a dependency of cz-conventional-changelog?

@AndersDJohnson AndersDJohnson force-pushed the engine-with-commit-types branch from b7a6ca0 to 56575d1 Compare August 11, 2016 02:03
@AndersDJohnson
Copy link
Member Author

@LinusU Thanks, I've force pushed a refactor that:

  1. makes engine independent of types, with no default
  2. uses a deep clone to ensure no modification of options object
  3. reduces logic to largely a single map call

// fine.
module.exports = function (options) {

options = cloneDeep(options || {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we never modify options we don't need to clone it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@LinusU Sure, you seemed (rightly) concerned about modification, so this would protect us against any future changes that could modify.

@AndersDJohnson AndersDJohnson force-pushed the engine-with-commit-types branch from 56575d1 to c8452d1 Compare August 13, 2016 19:36
@LinusU
Copy link
Contributor

LinusU commented Aug 14, 2016

Thank you for the awesome contribution 🙌

I'm merging this now 👍

I don't know exactly how the transferring of the repo works, I think that you can go into settings and then transfer it. Then @jimthedev would have to accept it, and I think add you as a maintainer as well. Can you try that?

@LinusU LinusU merged commit 7ea76be into commitizen:master Aug 14, 2016
@AndersDJohnson
Copy link
Member Author

@LinusU Thanks!

I've just invited you and @jimthedev as collaborators on the GitHub repo. Then I tried to transfer to commitizen organization, but can't since I'm not an admin there. If you want to add me to the org as admin, otherwise maybe I can transfer the repo to one of you first.

I added @jimthedev as an owner on the npm package, but I don't know your npm username, if you have one.

@LinusU
Copy link
Contributor

LinusU commented Aug 14, 2016

I'm linusu on npm :)

I'll see if I can transfer the repo now edit: I can't, "Settings" doesn't show up for me in the repo...

@AndersDJohnson
Copy link
Member Author

@LinusU Added you as npm owner.

For the repo, seems like either I'll have to transfer it to an admin of commitizen organization (you? @jimthedev?) who can then promote it, or you'll have to make me an admin of the org.

@jimthedev
Copy link
Member

@LinusU You're now a co-owner of the organization. @adjohnson916 invite sent to become a member. @LinusU should have whatever access is needed to grant you the appropriate rights.

@AndersDJohnson
Copy link
Member Author

Thanks @jimthedev! Well @LinusU I still cannot transfer repo to commitizen as I'm not an admin thereof. Shall I hand it off to you or Jim to promote, or do you want to make me an admin?

@AndersDJohnson
Copy link
Member Author

Sorry, when I try to transfer, it still says "You don’t have admin rights to commitizen." Wonder if I have to be an owner?

@LinusU
Copy link
Contributor

LinusU commented Aug 16, 2016

Funny that it should be so hard to transfer a repo 😆

Can you transfer it to me personally and I'll transfer it to the commitizen org and give you full rights to it? Thanks

@AndersDJohnson
Copy link
Member Author

@LinusU I know, right? Transferred it to you.

@LinusU
Copy link
Contributor

LinusU commented Oct 16, 2016

I'm so sorry for the delay on this, apparently I only had 24 hours to accept the transfer, and after I missed that deadline I completely dropped the ball on this. Sorry about that, if you want to try again I'll try to be more response :)

@AndersDJohnson
Copy link
Member Author

@LinusU No worries. Transferred again. Please move the repo to the commitizen org. And maybe consider adding me as an admin there. Thanks!

@LinusU
Copy link
Contributor

LinusU commented Oct 17, 2016

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.

4 participants