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(core): use git config to determine author before username #920

Merged
merged 8 commits into from
Jul 1, 2019
Merged

feat(core): use git config to determine author before username #920

merged 8 commits into from
Jul 1, 2019

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented Jun 7, 2019

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

This PR use git config to determine author before username. Since git repository has been initialized before initializing npm, we could get git author name and email.

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Can you please describe your use case in the pull request summary (under "Summarize your changes")?

packages/api/core/src/api/init-scripts/init-npm.ts Outdated Show resolved Hide resolved
packages/api/core/src/api/init-scripts/init-npm.ts Outdated Show resolved Hide resolved
packages/api/core/src/api/init-scripts/init-npm.ts Outdated Show resolved Hide resolved
@malept
Copy link
Member

malept commented Jun 7, 2019

Also, the commit message should be feat(core): use git config to determine author before username (note that it's core instead of cli).

@pd4d10
Copy link
Contributor Author

pd4d10 commented Jun 8, 2019

@malept , Thanks for the suggestion. Issues resolved and pushed.

The only thing I'm not sure is whether to use name <email> or object pattern. Seems npm init and yarn init both use name <email>. Could you confirm it again?

@pd4d10 pd4d10 changed the title feat(cli): try to get author info from git config feat(core): use git config to determine author before username Jun 8, 2019
malept
malept previously requested changes Jun 8, 2019
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

The only thing I'm not sure is whether to use name <email> or object pattern. Seems npm init and yarn init both use name <email>. Could you confirm it again?

It makes sense that those init subcommands use the string version, because it uses interactive prompts to get the author data. This is doing it programmatically, so we don't need to have the package manager parse strings.

packages/api/core/src/api/init-scripts/init-npm.ts Outdated Show resolved Hide resolved
packages/api/core/src/api/init-scripts/init-npm.ts Outdated Show resolved Hide resolved
@malept
Copy link
Member

malept commented Jun 9, 2019

I did some refactors on the code, but I think the last thing it needs are some tests.

@malept malept dismissed their stale review June 18, 2019 15:40

I made a bunch of changes

@malept malept requested a review from MarshallOfSound June 18, 2019 15:40
@malept
Copy link
Member

malept commented Jun 18, 2019

@MarshallOfSound could you take a look at this before it gets merged? The tests could probably be written better.

packages/api/core/src/util/determine-author.ts Outdated Show resolved Hide resolved
@malept malept dismissed MarshallOfSound’s stale review June 27, 2019 00:56

Applied change as requested

@malept malept merged commit 57e30a4 into electron:master Jul 1, 2019
dsanders11 pushed a commit that referenced this pull request Jan 14, 2023
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.

3 participants