search

approvals / go-approval-tests

Apache License 2.0
86 stars 22 forks source link

Initial support for scrubbers via a fluent API #37

Closed josephwoodward closed 2 years ago

josephwoodward commented 2 years ago

This should be small

Small pull requests are great and easy for me to understand and accept Please try prefix every commits in the pull request with Arlo's git notation Prefix Meaning
e development enviroment only - not production
d documentation only
t tests only
R!! Refactoring
B!! Bug Fix
F!! New Feature

But it's not small!

Then you should setup a remote pairing session with Llewellyn ( llewellyn.falco@gmail.com ) Usually the sessions are between 45-90 minutes.

assuming you still feel it is small, please include

Description

This pull request adds support for scrubbers via an approvals.Options() API. For example:

approvals.Options().WithRegexScrubber(scrubber, "<time>")

The solution

This is a new feature, so not much change needed to go into the existing API surface.

Notation

I prefer lots of very small commits prefixed with Arlo's git notation

hjwk commented 2 years ago

looks nice :)

An update to the readme to document the new functionality might be useful too ?

emilybache commented 2 years ago

I like the look of this as well, thankyou! I know you've been discussing more with @objarni so I'll wait for his comments too.

objarni commented 2 years ago

I'll pair review/update this PR with @JosephWoodward when he's back from vacation.

josephwoodward commented 2 years ago

@emilybache Thanks, as @objarni has mentioned, we're planning on a pair reviewing session to go through the pull request in finer detail.

isidore commented 2 years ago

@JosephWoodward I like a lot of this, but have some issues with the API. I would love to schedule a time to pair on it together. I feel we could address everything and release in an hour or so.

When's good for you?

josephwoodward commented 2 years ago

@isidore I'm quite flexible, do you have a calendar I can see? Do evenings work for you? @objarni and I had something in the calendar but things have come up meaning I've had to cancel which has been a shame as it'd be great to get this finished off.

objarni commented 2 years ago

Not sure if this happened?

I'm available Sunday CEST9:30-11 this weekend, would that suit you @JosephWoodward ?

emilybache commented 2 years ago

Shall I merge this now? Is everything ready?

objarni commented 2 years ago

Some functions still need options argument, and we want to rewrite the VerifyWithExtension function to use options instead, deprecating it. We have scheduled another session later this week.

objarni commented 2 years ago

(but it is already valuable to @JosephWoodward I believe)

josephwoodward commented 2 years ago

Pair programmed with @objarni but have yet to update the main documentation. Thanks @objarni for the really enjoyable pairing sessions!

This update now includes:

objarni commented 2 years ago

@emilybache @isidore it seems I don't have write access to the repository so cannot merge.

objarni commented 2 years ago

@JosephWoodward thanks for contributing and fun pair programming sessions!

emilybache commented 2 years ago

Thankyou so much for this! Looks great.