Skip to content

Get it working#2

Merged
damccorm merged 8 commits intomasterfrom
features/first-interaction
Aug 7, 2019
Merged

Get it working#2
damccorm merged 8 commits intomasterfrom
features/first-interaction

Conversation

@damccorm
Copy link
Copy Markdown
Contributor

@damccorm damccorm commented Aug 2, 2019

Should work on issues and pull requests. Allows you to display a message on the first one of either.

* Get it working

* Required token

* Logging

* Debug

* Debug

* Correct logging

* No setNeutral

* debug

* debug

* debug

* debug

* debug

* debug

* debug

* debug

* debug

* debug

* debug

* working

* logging

* logging

* logging

* logging

* logging

* logging

* logging

* logging

* logging

* logging

* debug

* debug
@damccorm damccorm closed this Aug 4, 2019
@damccorm damccorm reopened this Aug 5, 2019
@chrispat
Copy link
Copy Markdown

chrispat commented Aug 5, 2019

The readme needs an example of how to use the action.

@damccorm
Copy link
Copy Markdown
Contributor Author

damccorm commented Aug 5, 2019

Done

@damccorm
Copy link
Copy Markdown
Contributor Author

damccorm commented Aug 7, 2019

I think I've responded to all feedback, let me know if you think this is good to merge.

description: 'Input to use'
default: 'world'
inputs:
repo-token:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: Are we going with a pattern of abbreviating names? repo-token vs. repository-token. Personal preference is longer version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

repo-token is currently the standard across the board. I'm not inclined to change it at this point since it affects other actions as well (plus I just prefer repo haha)

throw new Error('Internal error, no sender provided by GitHub');
}
const sender: string = context.payload.sender!.login;
const issue: {owner: string; repo: string; number: number} = context.issue;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We can fix in a future PR but it's a bit confusing to call both "issue" and "pr" an "issue"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree, that's how octokit does it though. I think I agree that its worth changing though because its still confusing, but also that it can be done in a future PR!

@stephenmichaelf stephenmichaelf self-requested a review August 7, 2019 14:24
@damccorm damccorm merged commit 46c4697 into master Aug 7, 2019
@damccorm damccorm deleted the features/first-interaction branch August 7, 2019 14:27
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.

3 participants