Skip to content
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

Support Named Variables. Add appendVariable, getVariable, runSshScript, vibrate, waitToReturn actions. #8

Merged
merged 7 commits into from Nov 21, 2018

Conversation

Archez
Copy link
Contributor

@Archez Archez commented Nov 20, 2018

Checks

  • I've checked there are no linting errors.
  • I've checked the code is still at 100% test coverage.

Added Actions (if relevant)

  • appendVariable
  • getVariable
  • runSshScript
  • vibrate
  • waitToReturn

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!

@joshfarrant
Copy link
Owner

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!

@joshfarrant
Copy link
Owner

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:

  • Change appendVariable to addToVariable.
  • Change runSshScript to runScriptOverSsh.
  • Change vibrate to vibrateDevice.

I hope you don't mind me suggesting these changes, as other than those very minor things it's absolutely perfect (and testUUID looks great)! 🎉

@Archez
Copy link
Contributor Author

Archez commented Nov 20, 2018

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?

src/utils/testUUID.ts Outdated Show resolved Hide resolved
src/actions/getVariable.ts Outdated Show resolved Hide resolved
@joshfarrant
Copy link
Owner

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.

@Archez
Copy link
Contributor Author

Archez commented Nov 20, 2018

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.

@Archez
Copy link
Contributor Author

Archez commented Nov 20, 2018

Hmmm TravisCI does not like that I changed the casing for runScriptOverSSH..

@xAlien95
Copy link
Contributor

Hmmm TravisCI does not like that I changed the casing for runScriptOverSSH..

@Archez, file names haven't been changed, I suppose that's the problem.

@Archez
Copy link
Contributor Author

Archez commented Nov 20, 2018

Working off a mac. Didn't know macos does not inform git of case changes. Fixed using git mv.

@joshfarrant joshfarrant merged commit 048192a into joshfarrant:master Nov 21, 2018
@joshfarrant
Copy link
Owner

Fantastic, thanks again @Archez

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants