-
Notifications
You must be signed in to change notification settings - Fork 101
Support Named Variables. Add appendVariable, getVariable, runSshScript, vibrate, waitToReturn actions. #8
Conversation
Thanks for taking the time to do this! I'll have a proper look in a couple of hours, but at first glance it looks fantastic! To be completely honest I'd been putting off adding support for named variables. I'd spent a month or so building the structure of the project up before I released it, and so I needed a bit of a break from that side of things. You've done the project a real favour by adding that in! |
I've had a proper look through now, and it all looks great! The only modifications I'd ask for are the very minor things in the comments I've added, as well as some tweaks to the naming of some of the actions. Up to this point I've been camel casing the name of the action exactly as it appears in the Shortcuts app. I completely forgot to add this info to the Contributing Guide so that's on me, sorry about that! If you get chance, could you change the following:
I hope you don't mind me suggesting these changes, as other than those very minor things it's absolutely perfect (and |
Glad you liked it! I'll go ahead and make the necessary changes. That naming convention definitely makes sense now (I had done a few other actions that you added yesterday, but our names were different). I do not see any comments on the PR, are you sure you submitted them? |
Ah sorry, it looks like I'd started a review but that didn't actually create it. You should see them now. Sorry we ended up working on the same thing there, I don't know what the best way to handle that is going to be going forward. I'm open to any suggestions on how best to prevent it happening again. |
In response to your question about the ease of contributing: It felt fairly easy to get into the grove. I enjoy that you started with TypeScript (praise be!), and after digging through the interfaces/types I began to understand the overall structure. Took a minute to think about supporting name variables, but after that everything began to fall in line. |
Hmmm TravisCI does not like that I changed the casing for |
@Archez, file names haven't been changed, I suppose that's the problem. |
Working off a mac. Didn't know macos does not inform git of case changes. Fixed using |
Fantastic, thanks again @Archez |
Checks
Added Actions (if relevant)
Are you happy to be listed as a contributor in the actions list here?
Yes
Any other information / comments
I hope the testUUID util was added the way you would expect. I also sorted lines alphabetically in some spots.
I love this project and look forward to contributing more!