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

Add supports for custom replacer #185

Merged
merged 17 commits into from Apr 26, 2019

Conversation

halkeye
Copy link
Contributor

@halkeye halkeye commented Apr 24, 2019

super related to #168

Allows custom regexes to be applied.

Copy link
Member

@TimonVS TimonVS left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @halkeye 🙇‍♂️. I left some minor feedback but overall it looks good to me 🙂.

index.js Outdated Show resolved Hide resolved
lib/releases.js Outdated Show resolved Hide resolved
test/index.test.js Outdated Show resolved Hide resolved
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

Looks good so far, I have a few concerns

lib/releases.js Show resolved Hide resolved
lib/releases.js Outdated Show resolved Hide resolved
lib/releases.js Outdated Show resolved Hide resolved
lib/template.js Outdated Show resolved Hide resolved
@halkeye
Copy link
Contributor Author

halkeye commented Apr 25, 2019

Okay, thanks for the review, I think i got everything brought up handled.

@halkeye
Copy link
Contributor Author

halkeye commented Apr 25, 2019

travis needs a retry

lib/template.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

Thanks so much @halkeye — looks great! 👌🏼 First PR too! 🎉

I have some readme edits, and then we're good to go.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@halkeye
Copy link
Contributor Author

halkeye commented Apr 26, 2019

Thanks for all the comments and suggestions @toolmantim

I think i addressed them all, as well as fought with the master merge, so I think its ready to go again.

@toolmantim toolmantim merged commit 6394c7c into release-drafter:master Apr 26, 2019
@toolmantim
Copy link
Collaborator

Thanks for the updates, you’re awesome! 🎉🎉🎉🎉

@halkeye
Copy link
Contributor Author

halkeye commented Apr 26, 2019

:D SO EXCITED

How often do you release it to the actual github app? when can i start using it?

@toolmantim
Copy link
Collaborator

When the new npm package gets released, it gets deployed to match. I think the only thing we need to check is whether @TimonVS reckons we should add #188 beforehand?

@TimonVS
Copy link
Member

TimonVS commented Apr 27, 2019

I'm leaning towards adding the sorting option first because we're changing the default sorting direction which might be confusing to users. But I don't have a strong opinion about it so I'll leave the decision to you @toolmantim, I'll probably have some time tomorrow to start implementing the sorting option :)

try {
if (replacer.regex) {
return {
search: regexParser(replacer.regex),
Copy link
Member

@jetersen jetersen Apr 27, 2019

Choose a reason for hiding this comment

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

cheeky beep 😆 I spent an hour thinking my schema addition was borked when in reality you modified the schema. 😡 At myself for not reading the code more closely before deleting 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.

None yet

4 participants