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(pre-commit): check for author e-mail domain to match @weahead.se #5

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

michaellopez
Copy link
Member

@michaellopez michaellopez commented Jan 1, 2020

This PR adds a check to pre-commit phase to check that the domain for the commit authors e-mail address matches @weahead.se

There have been too many occurrences of devs commiting with their non-weahead e-mail addresses. This should help mitigate that going forward. After this is merged and published we should go over each repository and rewrite all the commits author e-mails to be their @weahead.se counterparts.

Some things to discuss before merge:

  • Should the error message be more "urgent" (red, bold, something else)? See screenshot Screenshot 2020-01-01 at 18 43 40
  • First pass of the error message I included the "replace Your Name with your real name" part, but I think that your developers should be smart enough to get that without that explanation, right?
  • Is the error message good enough?

@lofgrenfredrik
Copy link
Member

Nice addition!

  • Yes I think some form of highlight would be nice to emphasise the error message. Maybe just bolding it would be enough?

  • Yes I don't think that extra part would be necessary.

  • I think it's good enough, if not we can always update and tweak it as necessary.

@michaellopez
Copy link
Member Author

Don't forget that we need to handle the case of external collaborators that need to commit to the repos.

@lofgrenfredrik
Copy link
Member

@michaellopez
@linusklingzell had a good point regarding this.
Since our packages and configs are open to all we might want to make the email check toggleable? or in some other way optional.

@lofgrenfredrik
Copy link
Member

lofgrenfredrik commented Jan 10, 2020

@michaellopez
Maybe we can use the config key in package.json where you add an array of email domains you want to allow commits from?
Or have it create a config file where you add the email domains, specific emails or if we can use some form of glob pattern?
That way it's not locked to just weahead and it's easy to add emails for external collaborators.

@michaellopez
Copy link
Member Author

@lofgrenfredrik Yes, that sounds like a good plan. Let's try 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.

2 participants