Skip to content
This repository has been archived by the owner on Aug 8, 2024. It is now read-only.

Proposals on variables #10

Open
xAlien95 opened this issue Nov 22, 2018 · 17 comments
Open

Proposals on variables #10

xAlien95 opened this issue Nov 22, 2018 · 17 comments
Labels
discussion Discussion of future features

Comments

@xAlien95
Copy link
Contributor

I'm in the process of presenting a fork in which variables (named variables, magic variables and global variables (Ask, Clipboard, CurrentDate and ExtensionInput)) are objects instead of strings. In my local repository, I wrote:

const ActionOutput = (name?: string): Attachment => ({
  OutputUUID: uuidv4(),
  OutputName: name,
  Type: 'ActionOutput',
});

const Variable = (name: string): Attachment => ({
  VariableName: name,
  Type: 'Variable',
});

const Ask: Attachment = { Type: 'Ask' };
const Clipboard: Attachment = { Type: 'Clipboard' };
const CurrentDate: Attachment = { Type: 'CurrentDate' }; // TODO date/time formats
const ExtensionInput: Attachment = { Type: 'ExtensionInput' };

Using lowerCamelCase, an ask global variable would conflict with the ask() action function, so I ended up distinguishing them using CapitalCamelCase for variable objects. It could make sense since in my fork ActionOutput and Variable acts like JS types/classes:

const {
  buildShortcut,
  withVariables,
  ActionOutput,
  Clipboard,
} = require('@joshfarrant/shortcuts-js');
const {
  calculate,
  comment,
  number,
  showResult
} = require('@joshfarrant/shortcuts-js/actions');

// We'll use this later to reference the output of a calculation
const calcVar = ActionOutput('My result');

// Define our list of actions
const actions = [
  comment({
    text: 'Hello, world!',
  }),
  number({
    number: 42,
  }),
  calculate({
    operand: 3,
    operation: '/',
  }, calcVar),
  showResult({
    /**
     * We can use the result of the calculation in this Shortcuts's input
     * by passing the string to the 'withVariables' tag function
     */
    text: withVariables`Total is ${calcVar}! ${Clipboard}`,
  }),
];

Unfortunately the current lint script accepts variable names only in lowerCamelCase or UPPER_CASE.
How should I name the Ask global variable?

@xAlien95 xAlien95 changed the title Distinguish ask() action to ask global variable Distinguish ask() action from ask global variable Nov 22, 2018
@Archez
Copy link
Contributor

Archez commented Nov 22, 2018

I've also been thinking this out in my head. For the naming of the global variables, remember that the contribution guide suggests the display name from the shortcuts app itself (in English). My approach is quite similar to yours, but with a couple differences as described below:

// Global Variables
const askWhenRun: WFSerialization = {
  Value: {
    Type: 'Ask',
  },
  WFSerializationType: 'WFTextTokenAttachment',
};
const clipboard: WFSerialization = {
  Value: {
    Type: 'Variable',
  },
  WFSerializationType: 'WFTextTokenAttachment',
};
const currentDate: WFSerialization = {
  Value: {
    Type: 'CurrentDate',
  },
  WFSerializationType: 'WFTextTokenAttachment',
};
const shortcutInput: WFSerialization = {
  Value: {
    Type: 'ExtensionInput',
  },
  WFSerializationType: 'WFTextTokenAttachment',
};

// Util methods
const /** or makeMagicVariable */ actionOutput = (name?: string): WFSerialization => ({
  Value: {
    ...(name && { OutputName: name }),
    OutputUUID: uuidv4(),
    Type: 'ActionOutput',
  },
  WFSerializationType: 'WFTextTokenAttachment',
});
const formatSerialization = (variable: WFSerialization | string): WFSerialization => {
  // Named Variable
  if (typeof variable === 'string') {
    return {
      Value: {
        Type: 'Variable',
        VariableName: name,
      },
      WFSerializationType: 'WFTextTokenAttachment',
    };
  }

  // Already Serialized
  return variable;
};
const withVariables = (
  strings: TemplateStringsArray,
  ...vars: string[]
): WFSerialization => ({
  WFSerializationType: 'WFTextTokenString',
  Value: {
    string: strings.join('\ufffc'), // Join strings with object replacement character
    attachmentsByRange: strings.reduce(
      (a, _, i, s) => {
        // ...

        return {
          ...a,
          [`{${lengthSoFar}, 1}`]: formatSerialization(vars[i]).Value,
        };
      },
      {},
    ),
  },
});

Here I pulled the formatSerialization function from getVariable as a util function and made adjustments.

What this allows us is that any variable can be passed into withVariables and it will use just the Value object as WFTextTokenString serialized objects, while also allowing some actions to be able to take in the WFSerialization variable directly as a parameter for actions that support WFTextTokenAttachments.

Later on we can improve formatSerialization to be able to handle parameterizing variables (e.g. date formats, or changing the what input type to parse the variable as.

I'm curious what you think of my approach and any questions you have just ask.

@xAlien95
Copy link
Contributor Author

For the naming of the global variables, remember that the contribution guide suggests the display name from the shortcuts app itself (in English).

askWhenRun and shortcutInput look way better.

... allowing some actions to be able to take in the WFSerialization variable directly as a parameter for actions that support WFTextTokenAttachments.

I see, boolean values in dictionaries currently support WFTextTokenAttachments.

const formatSerialization = (variable: WFSerialization | string): WFSerialization => {
  // Named Variable
  if (typeof variable === 'string') {
    return {
      Value: {
        Type: 'Variable',
        VariableName: name,
      },
      WFSerializationType: 'WFTextTokenAttachment',
    };
  }

  // Already Serialized
  return variable;
};

If formatSerialization won't be used anywhere else, could it be renamed to variable(name: string) to become the equivalent of actionOutput()/magicVariable() for named variables? This way users will have to always use those functions to initialize variables and magic variables:

const result = magicVariable()
const myResult = magicVariable('My Result')

const list = variable('List')

@Archez
Copy link
Contributor

Archez commented Nov 22, 2018

In that scenario where we have variable(), do you think withVariables should still have the ability to take in strings (producing the named variable), or enforce that all named variables have to be initialized? I think I like the latter, as it locks things down, making sure the user has one intention. Similarly we can lock down setVariable/addToVariable to take in WFSerialization instead of string, and just have the actions strip off VariableName for the WFVariableName.

@joshfarrant What do you think?

@xAlien95
Copy link
Contributor Author

... we can lock down setVariable/addToVariable to take in WFSerialization instead of string, ...

That's exactly what I did. It will remove every check/test and it will make it easier to write the shortcut2js converter (#4) later on.

Using WFSerialization as base, the getVariable action just becomes:

const getVariable = (
  {
    variable,
  }: GetVariableOptions,
): WFWorkflowAction => ({
  WFWorkflowActionIdentifier: 'is.workflow.actions.getvariable',
  WFWorkflowActionParameters: {
    WFVariable: variable,
  },
});

@joshfarrant
Copy link
Owner

I think I like the latter, as it locks things down, making sure the user has one intention.

@Archez I'd tend to agree with you in leaning towards the latter. Having withVariables take in strings for named variables can end up in somewhat counterintuitive code.

withVariables`Testing ${'Some Variable'}`;

Could very easily be assumed to be equal to

withVariables`Testing Some Variable`;

Which is clearly is not.

Initialising the named variables beforehand, while it will add a bit more boilerplate, will make the code easier to understand, and the user's intentions clearer.

I really like the direction this is going in, definitely a big improvement over my initial implementation!

@Archez
Copy link
Contributor

Archez commented Nov 22, 2018

That is exactly my thought. The boiler plating aligns more with a 'code' structure rather than a 'iOS Shortcut' structure and also paves the way for a more concrete .shortcut -> .js parser.
Can't wait to see the results @xAlien95!

@joshfarrant
Copy link
Owner

Fantastic! Me too @xAlien95! 🧞‍♂️

@xAlien95
Copy link
Contributor Author

I pushed it to my fork, you can see and try it there.

@joshfarrant, do I have to split /src/utils/variables.ts in multiple files?
variable() and actionOutput() can be considered as util functions so it would make sense to store them in two separate files, but how should the global variables constants be stored in your opinion?

@Archez, should I use WFSerialization instead of Attachment as in your approach?

Note: I really don't know why my real name appeared twice on the commit.. It's the first time I'm using Atom to push commits. Any suggestion on how to prevent this behavior is highly appreciated!

@Archez
Copy link
Contributor

Archez commented Nov 23, 2018

If we do not use the WFSerialization approach, then any action that can take in a WFTextTokenAttachment (e.g. number), would have to be responsible for ensuring that the proper serialization step is taken. On the other hand, if we take the WFSerialization approach, then actions do not have to know the difference between a regular variable and a withVariables serialized object, allowing the actions code to be straight forward. I guess it would be a choice between handling it one (initialized as WFSerialization) or handle per basis (actions build the serialized object). I'm leaning towards WFSerialization, but I am open to objections. I can not imagine it being an issue either way, as user made scripts would be unchanged.

For your note: I just use VSCode (its great just like Atom), and haven't had strange issues like that using it's built in git tools.

@xAlien95
Copy link
Contributor Author

@Archez, I'm not familiar with TypeScript, but if we use the WFSerialization approach we may need to always specify that the .Value property is a SerializationValue. TypeScript automatically assumes it's a string although it can have multiple types.

I split ../interfaces/WF/WFSerialization.ts in two files and I updated withActionOutput.ts (rebased version of withUUID.ts) like this:

import SerializationValue from '../interfaces/WF/SerializationValue';
import WFSerialization from '../interfaces/WF/WFSerialization';
import WFWorkflowAction from '../interfaces/WF/WFWorkflowAction';

export const withActionOutput = <OptionsType>(
  actionBuilder: (options: OptionsType) => WFWorkflowAction,
) => (
  (
    options: OptionsType,
    output?: WFSerialization,
  ): WFWorkflowAction => {
    const action = actionBuilder(options);

    if (output) {
      const value = <SerializationValue>output.Value;
      action.WFWorkflowActionParameters.UUID = value.OutputUUID;
      if (value.OutputName) {
        action.WFWorkflowActionParameters.CustomOutputName = value.OutputName;
      }
    }
    return action;
  }
);

Is there a better way to achieve it?

@joshfarrant
Copy link
Owner

joshfarrant commented Nov 25, 2018

if we use the WFSerialization approach we may need to always specify that the .Value property is a SerializationValue. TypeScript automatically assumes it's a string although it can have multiple types.

That's strange, it shouldn't prefer one type over any other, as far as I know.

As I'm sure you've seen, the WFSerialization interface is defined here, and it's Value should expect a SerializationValue.

This is the first TypeScript project I've built from the ground up, so there may well be a better way of defining the WFSerialization interface. Don't know how much TS experience you have @Archez, but do you have any thoughts on this?

EDIT: I should also mention that I agree with @Archez in leaning towards WFSerialization over Attachment, if possible.

@joshfarrant
Copy link
Owner

joshfarrant commented Nov 25, 2018

do I have to split /src/utils/variables.ts in multiple files?
variable() and actionOutput() can be considered as util functions so it would make sense to store them in two separate files, but how should the global variables constants be stored in your opinion?

I'd say have variable and actionOutput in their own files.

I'm wondering whether it might make sense to have another directory, src/variables, to store everything related to variables. Or maybe we'd keep variable and actionOutput as named exports from the main src/index.ts (and keep them stored in src/utils), and just have askWhenRun, clipboard, etc defined in a single src/variables.ts file.

That way, users would have something like:

import {
  buildShortcut,
  withVariables,
  variable,
  actionOutput,
} from '@joshfarrant/shortcuts-js';

import {
  askWhenRun,
  clipboard,
} from '@joshfarrant/shortcuts-js/variables';

Thoughts?

@xAlien95
Copy link
Contributor Author

xAlien95 commented Nov 25, 2018

I'm wondering whether it might make sense to have another directory, src/variables, to store everything related to variables.

Later on, it could be useful to have a src/variables directory. I'm still doing researches on the Aggrandizements property and I'm starting to think that a Variable class with a .with() method could suit well:

const result = variable('Result');

const actions = [
  number({
    number: 1112223333,
  }),
  setVariable({
    variable: result,
  }),
  showResult({
    text: withVariables`Number is ${result}, phone number is ${result.with({
      type: 'PhoneNumber',
    })}!`,
  }),
];

... we'd keep variable and actionOutput as named exports from the main src/index.ts ...

I agree with having both variable and actionOutput to be available from '@joshfarrant/shortcuts-js'.

@joshfarrant, this issue is going off-topic from the original title. Should I rename it to something like "Proposals on variables", or close this one and open a new one?

@joshfarrant
Copy link
Owner

@xAlien95 .with() sounds interesting. I've not looked into the Aggrandizements property at all - is that essentially casting the type to a phone number then (in your example)?

I agree about renaming the issue, please go ahead 🙂

@xAlien95 xAlien95 changed the title Distinguish ask() action from ask global variable Proposals on variables Nov 25, 2018
@xAlien95
Copy link
Contributor Author

I've not looked into the Aggrandizements property at all - is that essentially casting the type to a phone number then (in your example)?

@joshfarrant yes (although a phone number cast does nothing at all, it just returns the number as a string since the number type doesn't have properties).

If you set a Text action with "Today at 9:00 am" and then you insert its output magic variable to be considered as a Date, you'll get the same time string you get typing "Today at 9:00 am" in the Date action. Aggrandizements can be avoided and replaced by actions, but they are useful if you don't want to break the action flow with actions like Get Dictionary Value: you can just cast a magic variable to a Dictionary and you'll be able to write the Key you want to obtain.

@xAlien95
Copy link
Contributor Author

xAlien95 commented Dec 2, 2018

@joshfarrant, step 2 of the variable implementation is live in my fork's variable-class branch.

  • Rebased variables on WFSerialization
  • Added Variable class with a .with() method for Aggrandizements
  • Added first implementation of some Aggrandizement types
  • Added PascalCase variable/class name validation in the linter (Decide on rules for naming Actions #26)

To do:

  • Update actions that can handle variables as parameter to have WFSerialization as additional parameter type
  • Update utils/buildSerialization.ts to handle withVariables tagged template strings
  • Add TypeDoc documentation for .with() method, with some examples

Contributors that have to implement actions that accept variables as parameters have to just add WFSerialization as additional type for that particular parameter (#27 (comment)), whilst users have to simply insert the variable:

getDeviceDetails({
  detail: askWhenRun,
})

Regarding .with(), its parameters are just the same that Shortcuts provides.
I put the coercion type under the type parameter:

Imgur

const myVar = variable('My Variable');

myVar.with({
  type: 'Dictionary',
  getValueForKey: 'My Key',
});

myVar.with({
  type: 'Date',
  dateFormat: 'Custom',
  customFormat: 'dd MMM at HH:mm',
});

myVar.with({
  type: 'Image',
  get: 'Album',
});

You can take a look at all the changes here: xAlien95@024d0ba.
I'm open to suggestions and considerations 😄
I'll add a pull request if there won't be issues regarding this implementation.

In step 3 I'll implement all the Aggrandizements, I already listed them all in this gist.
After that work, there shouldn't be other significative modifications to prevent us to start working on the .shortcut -> .js parser (#4).

@joshfarrant
Copy link
Owner

@xAlien95 Looks fantastic, great work. Can't wait to see the PR, feel free to submit whenever you're ready.

After that work, there shouldn't be other significative modifications to prevent us to start working on the .shortcut -> .js parser.

This is very exciting. Everything else is coming along well, so I might start taking a look into ways to approach that soon.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion Discussion of future features
Projects
None yet
Development

No branches or pull requests

3 participants