Piotr Krzemiński
07/29/2024, 8:22 AMVampire
08/10/2024, 12:02 AMVampire
08/10/2024, 12:06 AMUnTyped
or similar.
The latter would of course be a breaking change and require a new binding server version route. 😄Vampire
08/10/2024, 12:17 AMPiotr Krzemiński
08/10/2024, 7:52 PMThe problem with the binding server is, that it is a step backwards from the org title.IMO it depends which metric you take to judge this 🙂 mine are: (1) effort to use an arbitrary action even in the most unsafe manner and (2) effort to have some kind of type-safety. In the old world, for (1) one could use
CustomAction
API, and for (2) there is subclassing of RegularAction
. Thanks to the bindings server, the new lowest-effort method is IMO using the binding provided by the server, and it already gives you maybe not type-safety, but at least the inputs are mapped properly so it will catch certain kinds of errors. Taking this into account, IMO we're getting closer to a something safer by defaultPiotr Krzemiński
08/10/2024, 7:54 PMHow about some itching (...)I actually really like the idea because it would point the users to contributing to the typing catalog. I want the catalog to be co-owned by the community, and I'll do my best to maintain it (I'm in the middle of creating some tools to make it simpler, e.g. https://github.com/typesafegithub/github-actions-typing-editor). I'll think about what you proposed with the extra suffix!
Piotr Krzemiński
08/10/2024, 7:54 PMAlso, how do you test your typings?what do you mean exactly? what kind of test?
Vampire
08/11/2024, 1:10 AMI'll think about what you proposed with the extra suffix!Maybe not a good idea, as then the action suddenly gets renamed just because inputs changed. But having the implicitly typed properties deprecated, or the whole action if no typings present, would at least hint to the improvement potential without breaking things.
what do you mean exactly? what kind of test?Manual. You develop an action or an update for an action. You change also the typings for it. You could maybe there push to some fake tag and use that as dependency to test the action. But pushing for that might not be a good idea, but you might want to test the typings without pushing your action. And even worse, you develop typings for a 3rd party action, either for a PR or for the catalog. You either have to pull a fork and use the fork for testing, or blindly get things merged and test it only after that. Would be better if there were some way to test, for example like I described.
Piotr Krzemiński
08/11/2024, 6:56 AM_Untyped
binding if the action doesn't have any kind of typings provided
• generating both the _Untyped
binding and the type-safe binding if typings are available
This way not only we don't break consumers of actions that use the untyped bindings, but also provide a safer way of using an action if some of the inputs have to be defined as GitHub expressions (currently it has to be done using _customArguments
).
Regarding @Deprecated
, I don't like this approach if we were vending only untyped binding because there's no Kotlin entity immediately available that is not deprecated, so I feel the semantics of deprecation would be broken here. But I like the idea of marking as deprecated the untyped binding if there's also the typed binding. (edit: or actually maybe we shouldn't deprecate it to use it in the się case I described earlier, with the expression)Vampire
08/11/2024, 3:32 PMI thought about:
• generating just thebinding if the action doesn't have any kind of typings provided_Untyped
• generating both theThe breaking was more for the in-between case when the typing became outdated for example because some input was added, so thebinding and the type-safe binding if typings are available_Untyped
_Partilytyped
case. 😄
But I guess those should maybe just be made deprecated like the whole _Untyped
class.
You could also extend the concept on a propertly level of course, having for each property an ..._Untyped
and if a type was configured one without the suffix.
That would also care for misconfigured types as you can then use the ..._Untyped
property until it is fixed.
or actually maybe we shouldn't deprecate it to use it in the się case I described earlier, with the expressionHm, yeah, if the
_Untyped
concept expands to the property level, that might be enough.
Of course the deprecation would conceptually not be the right tool, just was the first that came to mind to notify the consumer that he should do something like making the action author add typings or get them into the catalog repository.
The _Untyped
might be enough as hint.
I also want it for me / power users, not only "simple consumers".
If I add a new action dependency, I don't want having to look in the action repo and the catalog whether there are typings and indeed everything is String
.
I want some in-workflow-file notification like deprecated or suffix that tells me something should be done eventually.
Maybe like this:
• Suffix the class and all properties with _Untyped
if no typing is available
• When any typing is available
◦ additionally have the unsuffixed class
◦ maybe mark the suffixed class as deprecated
◦ in the unsuffixed class have all properties with _Untyped
for not-yet typed things and expressions
◦ additionally add unsuffixed properties for the typed stuff
◦ maybe mark for String
typed properties the _Untyped
one as deprecated as also the typed one can be used, or maybe better not in case the type changes in the future, so maybe better always use _Untyped
for expressions even if the type is String
Would be better if there were some way to test, for example like I described.Or you could use a version like
v4__typing-data_base64encodedtypingdata
to supply the typing data that should be used for the generation.
That way you could even override typings you found to be wrong, or if you need a field String-typed to use an expression (ignoering the Untyped discussion here 😄)Vampire
08/11/2024, 4:47 PMPiotr Krzemiński
08/11/2024, 7:01 PMVampire
08/14/2024, 9:54 AMString
to int
, this will be a breaking change and existing workflows will break as the binding server will generate the new bindings.Piotr Krzemiński
08/14/2024, 9:56 AMVampire
08/18/2024, 2:56 PMVampire
08/18/2024, 2:57 PMPiotr Krzemiński
08/18/2024, 3:03 PM_Untyped
, isn't it the case? I have a test for it: https://github.com/typesafegithub/github-workflows-kt/blob/9f0928bf719915e1a97bf2e983fad7fd0c55b77c/.github/workflows/test-script-consuming-jit-bindings.main.do-not-compile.kts#L25-L26Piotr Krzemiński
08/18/2024, 3:04 PMVampire
08/18/2024, 3:12 PMclass FooAction_Untyped {
val foo_Untyped: String
val bar_Untyped: String
}
full typing:
@Deprecated
class FooAction_Untyped {
val foo_Untyped: String
val bar_Untyped: String
}
class FooAction {
val foo: Int
val foo_Untyped: String
val bar: String
val bar_Untyped: String
}
partly typed:
@Deprecated
class FooAction_Untyped {
val foo_Untyped: String
val bar_Untyped: String
}
class FooAction {
val foo: Int
val foo_Untyped: String
val bar_Untyped: String
}
Piotr Krzemiński
08/18/2024, 3:16 PMVampire
08/18/2024, 3:18 PMVampire
08/18/2024, 3:18 PMVampire
08/18/2024, 3:20 PMPiotr Krzemiński
08/18/2024, 3:23 PMVampire
08/18/2024, 8:59 PMexactly, that's why ideally the action owner should publish such change as a new major version.And another potential for breaking changes. We have typings in the catalog one way (for example boolean for the tmate action) and the action adds typing that disagrees (for example enum for the tmate action). You can hardly require the tmate action to make a major version bump just for adding typings that disagree what we have in the catalog 😄
Vampire
08/19/2024, 1:23 AMMaybe I'll find some minutes soon to implement what I envisioned.Well, was a bit more than some minutes, but thanks to the previous work on the type-safe output/input wiring, I got it done in full I think. Should be ready to review as soon as gt is re-enabled, as it is on top of my stack so that I could test it in the actual workflow file. 🙂
Vampire
08/19/2024, 1:25 AMPiotr Krzemiński
08/19/2024, 5:50 AM