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: Add ability to customize Clubhouse story title and body #10

Conversation

jdcargile
Copy link
Contributor

@singingwolfboy, this PR has a feature that includes the ability for users of this GitHub Action to customize the title and body of the created Clubhouse story using the same custom comment template you're using for the PR comments. If neither clubhouse-story-title-template nor clubhouse-story-body-template is populated, it simple defaults to the payload.pull_request.title and payload.pull_request.body as requested.

@jdcargile jdcargile changed the title Feature/customize clubhouse story title and body feat: Add ability to customize Clubhouse story title and body Sep 29, 2020
Copy link
Owner

@singingwolfboy singingwolfboy left a comment

Choose a reason for hiding this comment

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

I added a few comments for suggested changes, and this pull request needs some automated tests, as well. But I really like where this is going, and I think it will be a useful feature and ready to merge soon! 🎉

src/util.ts Outdated
Comment on lines 274 to 275
storyTitle: string,
storyBody: string
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than making this function accept arguments for storyTitle and storyBody, could you please construct those values inside the createClubhouseStory function? That keeps the opened.ts file clearer, and allows for easier testing, as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@singingwolfboy, the title and description for the story are now in the createClubhouseStory method.

src/opened.ts Outdated
"clubhouse-story-title-template"
);
const storyTitle = Mustache.render(CLUBHOUSE_STORY_TITLE_TEMPLATE, {
payload,
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than passing the payload object to the Mustache environment, I think it's clearer to pass the pull request object, like this:

{ pullRequest: payload.pull_request }

I doubt that template authors will need any other information from the payload. I'm undecided about pullRequest vs pull_request, but I feel like the former is more Javascripty, so that's why I'm leaning towards it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@singingwolfboy, I debated this too thinking pull_request would have everything I would want to use, but then when I tried to do what I was doing before of concatenating the repo.name to the Clubhouse story, it was missing in the payload.pull_request object.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, I didn't consider that! Alright, then I guess the payload object is the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Leaving payload whole as the passed attribute.

@@ -83,6 +83,35 @@ template output valid Markdown, as shown above.
If you don't provide a comment template, this action will use this comment template
by default: `Clubhouse story: {{{ story.app_url }}}`

## Customizing the Clubhouse Story Title and Body
Copy link
Owner

Choose a reason for hiding this comment

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

This is great! Thank you for adding this documentation 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

README.md Outdated
variables, like this:

```yaml
- uses: singingwolfboy/create-linked-clubhouse-story@v1.7
Copy link
Owner

Choose a reason for hiding this comment

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

Please leave this at v1.5 for now. I'll update all the version references in this file when I release a new version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! No problem!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jdcargile
Copy link
Contributor Author

@singingwolfboy, what did you have envisioned for the automated tests for the createClubhouseStory method. There is a fair amount of logic in that method that might need to be broken out to do the testing I see elsewhere in the project. Please provide a little more guidance on this.

@singingwolfboy
Copy link
Owner

Thanks for all your work on this! I think I'll just merge this, and write some tests myself.

@singingwolfboy singingwolfboy merged commit 4d02c07 into singingwolfboy:main Oct 3, 2020
@singingwolfboy
Copy link
Owner

Here are the tests I put together: 5130a7f

Getting everything to work right with nock was harder than I thought it would be!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants