https://kotlinlang.org logo
Join Slack
Powered by
Hm, I need `actions/cache/restore@v3` and `actions...
# github-workflows-kt
v
Hm, I need 
actions/cache/restore@v3
and 
actions/cache/save@v3
. But I fear the generator will not work with that? 😭
p
that's correct, it's currently not supported. But it should be pretty simple, I've created a feature request: https://github.com/krzema12/github-workflows-kt/issues/646
@Vampire what name of the wrapper class would you expect such nested actions to have? (I have my idea, but I'm wondering what you propose without suggestions)
v
joined with camel-case
CacheRestoreV3
CacheSaveV3
p
Ok, and what would you say for 
Cache_RestoreV3
? My intent is to show that this is a nested action, but now I'm not sure if it's worth showing. Thoughts?
v
So I guess you are currently enhancing the wrapper generator?
p
I'm planning to do so before the next release, didn't start yet
v
Hm, also an idea. Underscores just feel awkward in class names. 😄
Not sure, would probably also be fine
p
Yep, that's why it's a bit controversial :)
Ok, it's easy to change later, while still in unstable mode
Another idea: 
CacheV3.Restore
- 
Restore
being a nested class of 
CacheV3
Expresses nesting in a clean way IMO
v
Hm, also a good idea. But you have to make sure that 
CacheV3
can be a completely own action in itself, or that it can only be a holder.
p
Yes, true - adds to the complexity of the code generator a bit
v
What if the action is in 
my/littl/action/action.yaml
will it then be 
My.Little.ActionV0
?
Not objecting, just noting
p
Following my example, it would be 
MyV0.Little.Action
. Looks a bit weird, doesn't it? This version in the first part. But it matches the fact that the root thing is versioned
v
Ah, right
Yeah, and if there is actually something in the upper level too, it is also arguable that the version is there.
p
Sorry, didn't get it. Could you rephrase?
v
Yeah, weird sentence, sorry 😄
If My is an action itself and Little and Action are sub-things that are versioned together with the tag like for the cache action, it also seems ok to have the version there the front
Definitely needs some getting used to it, but maybe not too bad and probably definitely better than underscores 😄
p
Cool, so I think I'll go this path :) thanks for the brainstorming
v
Btw. is the type for outputs relevant In the context of your lib? I've e. g. seen in the cache types that you have string as output and a comment to check from the description and set accordingly. But even if I set it to 
boolean
which it should really be, the generated class does not change of course, the accessor only gives the expression string for it anyway.
p
It's the way it currently works, the outputs are basically untyped (string) GitHub expressions. The only benefit of having them generated is being able to rely on the output names, which gives some level of safety (probably not yet type-safety)
It's related to https://kotlinlang.slack.com/archives/C02UUATR7RC/p1674583564743569?thread_ts=1674583564.743569&cid=C02UUATR7RC in a sense that one day I'd love to have proper type-safety when passing values between outputs, inputs and other parts of the API
v
Yeah, as I said, I think you need denotable union types. Then you can declared the fields as taking a 
boolean
or a 
BooleanExpression
and the output would supply the latter which could also be used for the Gradle cache action example. In the meantime you could probably just have two properties or two setters each taking one of them. Or alternatively you could use 
Either
from Arrow or make an own version. But that would make the direct setting less nice.
p
my plan is to wait for some development of events in [KT-13108] Denotable union and intersection types . If it doesn’t happen (soon-ish), I considered both approaches you mention (several arguments per input, depending on the use case) or something similar to 
Either
, but with more specific “sides” like 
typed
and 
untyped
v
Yeah, thats the issue I talked about. But I somehow don't expect it to happen soon-ish, just anytime-ish. 😄
I guess in the light of the denotable union types for mid-term it would be better to have two separate properties. Then when denotable union types finally arrive the typed field value settings can stay the same and just the others need to be changed, probably by simply removing 
Expression
from 
myFieldExpression
. If you have something like a specific 
Either
you will break all usages when switching to denotable union types.
I've seen your new branch. Did you change mind and not make the "subactions" inner classes? Or is it just an intermediate step that just worked the quickest and you will change it? 🙂
p
I’ve slept on the the idea of having 
FooV1.Bar
and figured it would be pretty complex to implement in a proper, generic way + one day I’m hoping to create a complier plugin that would generate the wrappers, and I’m not sure how easy it would be to generate a single class with nested classes for more than 1 requested action wrappers. That’s why I decided to go with something simple that I almost have implemented. I prefer to put the effort in more crucial features
once it’s rolled out, I’m still open to feedback - maybe the simplified implementation creates some issues. For now the only problematic cases I can think of are e.g. clashes between 
some-action/foo/bar-baz
and 
some-action/foo-bar/baz
which will lead to generating the same class name, but I assess the risk of getting these as low
(I should’ve updated the ticket regarding changing my mind, sorry)
another argument is being in line with what you intuitively came up 🙂 https://kotlinlang.slack.com/archives/C02UUATR7RC/p1674589041434689?thread_ts=1674580678.412759&cid=C02UUATR7RC this intuition is important to me
v
Yeah, all is well, was just curious. Definitely no excuse needed, especially when you immediately start working on things I want to have. :-D
p
sure, but it’s good you ask - I like having design decisions documented 🙂 and I like being challenged (just not too much, to not lead to DoS)
v
I guess in the light of the denotable union types for mid-term it would be better to have two separate properties.
Maybe something like this will work nicely: https://github.com/Vampire/github-workflows-kt/commit/4ed978461cfbfeb4d9ae0ff2175f1f45de0f17b9 Not sure about the `List`s though
p
thanks, I’ll have a look later!
and back to the original topic, these two are merged and will be released next week: ‱ add actions/cache/save@v3 ‱ add actions/cache/restore@v3
v
and back to the side topic, here a bit more complete version with working generation, but completely untested: https://github.com/Vampire/github-workflows-kt/commit/87efab0e297cfceea5deebce256f75887101e331 At least it compiles and (with some modifications) the tests pass 🙂
Copy code
val foo = uses(action = SetupWslV1())
val bar = SetupWslV1(
    distributionExpression = foo.outputs["bar"],
    useCacheExpression = Expression("1 == 2"),
    updateExpression = foo.outputs.wslShellDistributionWrapperPath as Expression<Any>,
    wslShellCommandExpression = foo.outputs.wslShellDistributionWrapperPath
)
p
looks interesting 👀 I have it in my queue to look at it, but now I need to take several days off form the project, I'm out of budget because of this coding frenzy 😉
v
As you are back, did you have a look? 🙂 Btw. I'd prefer you adding tests for all that. I already put too much time into it. 😄 And it would allow to test the changes with a fresh look on it. 🙂
p
I’m not yet gone actually 😛 recent contributions kept be busy, I was planning to take a week off form working on the lib now when the release is out
test: some kind of tests are in place, let me know which ones you mean exactly
v
Oh, haha, sorry. 🙈 That change adds significant changes and functionality, I guess there should be some tests testing whether the type-safe wiring works like expected and so on. :-)
p
this feature competes with strategy matrix support - what would you say is more impactful to you?
fortunately the typed inputs is a simpler, incremental feature, so we can try to get it through sooner
v
typed outputs you mean probably. 😄 Now that you mention it, I guess the typed outputs does not compete with the matrix support, but is a prerequisite. What you get from the matrix is an expression, so if you want to use matrix values for non-string fields, you need at least something similar. It probably needs to be expanded even, so that you can use outputs or matrix values for other properties too. It currently only works for 
String
fields.
p
ideally we’d have a comprehensive design effort for this kind of type-related stuff, so that all is consistent with each other đŸ€” but yeah, time 😅
v
Which to tackle first best I don't know, but I also guess the typed outputs will be simpler and faster through
p
yes, a good way to start
40 Views
Open in Slack
PreviousNext
Powered by