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

Get it working #2

Merged
merged 8 commits into from
Aug 7, 2019
Merged

Get it working #2

merged 8 commits into from
Aug 7, 2019

Conversation

damccorm
Copy link
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
lib/main.js Outdated Show resolved Hide resolved
@damccorm damccorm closed this Aug 4, 2019
@damccorm damccorm reopened this Aug 5, 2019
@chrispat
Copy link
Member

chrispat commented Aug 5, 2019

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

@damccorm
Copy link
Contributor Author

damccorm commented Aug 5, 2019

Done

README.md Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
@damccorm
Copy link
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.

README.md Outdated Show resolved Hide resolved
description: 'Input to use'
default: 'world'
inputs:
repo-token:

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
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;

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
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