Skip to content

Improve checking when adding to PR cc: list#362

Merged
dscho merged 2 commits into
gitgitgadget:mainfrom
webstech:cc-with-space
Dec 2, 2020
Merged

Improve checking when adding to PR cc: list#362
dscho merged 2 commits into
gitgitgadget:mainfrom
webstech:cc-with-space

Conversation

@webstech

@webstech webstech commented Nov 19, 2020

Copy link
Copy Markdown
Contributor

The checking for existing entries in the cc: list will now:

  • allow a non-empty line (whitespace) before the footers
  • ignore case for both cc: entries and PR author email
  • support a body with only a footer
  • check only email and not names in cc: entries

The code assumes an empty line (may have whitespace) separates the body from the footers and no empty lines are in the footers.

Tests were added to validate the new checks.

This resolves a problem seen in 776 where users were copied multiple times and 783 where the PR author was copied.

Additionally this change will simplify GitHubGlue add CC testing:

Mock functions are now used for the tests. This removes the need to update the actual PR on GitHub, which is significantly faster.

@webstech webstech requested a review from dscho November 19, 2020 06:58
Comment thread lib/github-glue.ts Outdated
@webstech webstech changed the title Add cc - allow non-empty line before cc: Improve checking when adding to PR cc: list Nov 23, 2020
@webstech webstech requested a review from dscho November 23, 2020 05:38
Comment thread lib/github-glue.ts Outdated
Comment thread lib/github-glue.ts Outdated
Comment thread lib/github-glue.ts
@webstech webstech force-pushed the cc-with-space branch 2 times, most recently from 4114da5 to 9fbb322 Compare November 25, 2020 06:54

@dscho dscho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry to make it so difficult, but I think this still needs to be tweaked a bit more.

Comment thread lib/github-glue.ts Outdated
Comment thread lib/github-glue.ts Outdated
Comment thread lib/github-glue.ts Outdated
Comment thread lib/github-glue.ts
Comment thread tests/github-glue.test.ts Outdated
Comment thread tests/github-glue.test.ts Outdated
Comment thread lib/github-glue.ts Outdated
dscho
dscho previously approved these changes Dec 1, 2020

@dscho dscho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks pretty good! I like how sweet the unit test looks now (although I have to admit that I only skimmed it, trusting that it is correct).

The checking for existing entries in the cc: list will now:

- allow a non-empty line (whitespace) before the footers
- ignore case for both cc: entries and PR author email
- support a body with only a footer
- check only email and not names in cc: entries

Tests were added to validate the new checks.

Signed-off-by: Chris. Webster <chris@webstech.net>
Mock functions are now used for the tests.  This removes the need to
update the actual PR on GitHub, which is significantly faster.

Signed-off-by: Chris. Webster <chris@webstech.net>
@dscho dscho merged commit 9cc511a into gitgitgadget:main Dec 2, 2020
@dscho

dscho commented Dec 2, 2020

Copy link
Copy Markdown
Member

Thank you!

@webstech

webstech commented Dec 3, 2020

Copy link
Copy Markdown
Contributor Author

Thank you for your patience. Should have been less painful for you.

@dscho

dscho commented Dec 3, 2020

Copy link
Copy Markdown
Member

Oh, but it wasn't painful at all! Thank you for taking such a lot of GitGitGadget maintenance off of my shoulders ❤️

@webstech webstech deleted the cc-with-space branch December 4, 2020 23:03
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