-
-
Notifications
You must be signed in to change notification settings - Fork 16
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement set_cookie_item
action
#32
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.
Hey @lxxxvi, thank you very much for opening this pull request! 馃帀
I left some comments and ideas for refactoring, but I think that the functionality itself is awesome as-is!
It seems like that the project itself isn't building because some type annotations are missing. If you run yarn watch
in the terminal it should also give you the same errors the GitHub action added as annotations.
Let me know if you are stuck, I'm more than happy to help you getting them resolved 馃檶馃徏
6a1f6ba
to
1429233
Compare
@marcoroth Thanks for your review, I appreciate it! I applied the suggested changes, you may have second look now 馃槈 |
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.
This looks good to me!
Thanks for taking this on @lxxxvi! 馃帀
This pull request updates the stream helper for the `set_cookie_item` action. It makes the positional arguments optional in order to just allow to specify them as keyword arguments. this action was implemented in marcoroth/turbo_power#32
Hi 馃憢
This PR addresses #1 (wohoo).
I may have taken a "overcomplicated" approach to build the Cookie String. I appreciate your feedback, letme know what/if I can improve anything, I'm eager to learn! Thank you.