-
Notifications
You must be signed in to change notification settings - Fork 370
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
Add action typings #830
Add action typings #830
Conversation
@luketomlinson friendly ping 😄 |
@rentziass @flaxel @JoannaaKL could you take a look at this PR? |
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 addition to the project! 🎉
@flaxel ready for another round of review 😄 |
I took a look at some options and sometimes the type of the value didn't match. Maybe you can check again. 👀 |
@flaxel feel free to point of an example. |
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.
Oh sorry, I had two values mixed up. 🙈 Small note to me, no more work in the evening. 😆
@Newman1515 I don't understand why the "test" workflow step failed. I've run |
This reverts commit a105d4b.
In a PR in my fork (here: krzema12#1) I got a green build: |
@Newman1515 @krzema12 please check this PR, I debugged and fixed the issue with the |
@luketomlinson @rentziass @JoannaaKL @Newman151 friendly ping. |
@JoannaaKL @e-korolevskii could you take a look please? The PR on which this one waited for #859 has just got merged. |
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.
Thank you for this improvement!
Thanks! @JoannaaKL could you merge? I have no permissions. |
@Newman1515 please take a look. |
@marko-zivic-93 @IvanZosimov thanks for approving! Could you merge? |
@marko-zivic-93 @IvanZosimov ping. |
1 similar comment
@marko-zivic-93 @IvanZosimov ping. |
Hi, @krzema12 👋 We are still investigating your PR, sorry for the late response. As soon as we finish we get back to you with updates. |
Hi, @krzema12 👋 Sorry for the delay, we have investigated your PR. As we get it right, the |
@IvanZosimov sure! The goal of this PR is in fact to add a piece of documentation. However, it's not yet another piece of Markdown. When working with various actions, I discovered that it's challenging to know what type of data is needed for the inputs of an action, the same for outputs. Every action has its own custom way of documenting it, if it documents it at all, which makes it challenging for humans and automation to work with various actions. I figured there must be a way to address it. actions/stale has 52 (!) inputs and 2 outputs. It's the most complicated action API I've ever saw. That's why I thought it will be a good candidate to receive proper API documentation regarding types. I looked for an inspiration in other ecosystems where types were added later, like TypeScript adding types for JavaScript, or type hints in the Python world. I decided to create github-actions-typing which aspires to be the standardized way of typing your action's API. Onboarding is as easy as adding an extra Addressing your question specifically:
You correctly inferred that the goal of the action itself is to validate the typings, i.e. if each action input and output has a type assigned, and that the type is correct according to the standard. It works a bit like mypy in the Python world that allows adding static type checks over untyped API. |
@krzema12, Thanks for the reasonable clarification! Unfortunately, we have to reject your PR, and there are some reasons why we made this decision:
However, we are really thankful to you for your willingness to make our action better ❤️ |
@IvanZosimov may I ask how you expect automation to recognize the types from the prose in For example the |
@Vampire, Sorry, we fully understand your point, but at the moment, we aren't ready to accept this PR. |
Sad, but of course your decision. |
Changes
Context
GitHub's actions generally lack a standardized way to expose information about types of their inputs and outputs. It's hard both for the users and for automation that may want to rely on it. Thanks to https://github.com/krzema12/github-actions-typing/, types are provided in automation-friendly way. One of such pieces of automation is a Kotlin DSL for authoring GitHub workflows: https://github.com/krzema12/github-actions-kotlin-dsl
Ideally GitHub should provide a way of defining these in e. g. action.yml, but it's not supported yet. Adding typings using https://github.com/krzema12/github-actions-typing/ is an iterative step towards having the typings. Possibly one day first-class support from GitHub will be added, and until then the typings added in this PR can greatly help.
For more info, please read the project's README.