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

Remove references to payload as much as possible #1

Conversation

NathanielRN
Copy link

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

Copy link
Owner

@ktrz ktrz left a 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

src/extract.ts Outdated Show resolved Hide resolved
src/extract.ts Outdated Show resolved Hide resolved
src/extract.ts Outdated Show resolved Hide resolved
@NathanielRN NathanielRN force-pushed the replace-payload-repo-with-context branch from 0f3dd79 to 4ed7fef Compare October 2, 2021 15:46
@NathanielRN
Copy link
Author

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

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 github.context.payload was being used for it's repo.repo and repo.owner information in several places. However, this information already exists on every requests directly on the github.context object. For that reason, I just replaced the payload references because like you and I discovered, the payload does not exist on schedule and workflow_dispatch event types. Therefore this is a necessary change to make sure all the features of the action work with these event types.

Please let me know if that makes sense!

@NathanielRN NathanielRN requested a review from ktrz October 2, 2021 16:09
Copy link
Owner

@ktrz ktrz left a 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

src/extract.ts Outdated Show resolved Hide resolved
src/write.ts Outdated Show resolved Hide resolved
@NathanielRN NathanielRN force-pushed the replace-payload-repo-with-context branch from 4ed7fef to e6d78ac Compare October 4, 2021 02:38
@NathanielRN NathanielRN requested a review from ktrz October 4, 2021 02:44
@NathanielRN NathanielRN force-pushed the replace-payload-repo-with-context branch from e6d78ac to 61b16d2 Compare October 4, 2021 02:48
@ktrz
Copy link
Owner

ktrz commented Oct 5, 2021

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.

  • The first layer of abstraction is:
    if PR payload is available then get data from it and if not get it via REST API. (I don't need every detail of how that data is gathered at this point)
  • Second layer of abstraction:
    if I need to change/read how either getting data from payload or REST is handled then I can go into the appropriate function's code and read the details

@ktrz ktrz merged commit 3911100 into ktrz:fix/manual-and-scheduled-runs Oct 5, 2021
@NathanielRN NathanielRN deleted the replace-payload-repo-with-context branch October 5, 2021 18:52
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.

2 participants