-
Notifications
You must be signed in to change notification settings - Fork 0
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
Remove references to payload as much as possible #1
Remove references to payload as much as possible #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @NathanielRN
Thank you very much for your PR! I've added a couple of comments to your changes.
Could you please explain why do you want to remove references to payload
as much as possible? With my changes I assumed that it was used to avoid having to use REST API unnecessarily if the data is already provided with the event that caused the action to run
0f3dd79
to
4ed7fef
Compare
Yes that's a good goal! You'll see that with my changes, no REST API calls were added 🙂 Please let me know if I missed anything 😅 In this action, the Please let me know if that makes sense! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Now I can see your intended changes better!
Just left a couple of small comments
4ed7fef
to
e6d78ac
Compare
e6d78ac
to
61b16d2
Compare
Thank you @NathanielRN for your contribution and for applying my suggested changes! I'll merge it to the main repository soon and release the new version I love to learn new perspectives on code quality. From my perspective functions' role is not only to re-use code but also (and maybe primarily) to split logic so that the code can be read on different abstraction levels and also to follow SRP. eg here.
|
Description
Hey @ktrz ! Really appreciated your original PR on the upstream action repo because it unblocked me with a similar issue 😄 I took it a step further to remove all references to the
payload.repository
and updated the tests. Hopefully if you merge this and the upstream maintainer merges your PR we can all benefit from this fix! 🙂