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

feat: allow passing args to object methods #20

Closed
wants to merge 3 commits into from

Conversation

haltcase
Copy link

@haltcase haltcase commented Nov 14, 2016

Add the ability to pass (string) arguments to object methods in a space delimited format. From what I understand, this would preclude strict adherence to Python's string formatting where spaces would be valid in keys when resolving against the object. ( and I know this has been requested or is potentially planned )

Despite that, I needed the functionality for myself and thought I'd propose it. If nothing else anyone interested can use my fork. I totally understand if it's not merged here.

Examples

( see here for a jsfiddle )

Assume all examples below use this object:

var obj = {
  getFirstName (other) {
    if (other) return 'Billy'
    return 'Alec'
  },
  lastName: 'Baldwin'
}

Prior to this PR, simple object method calls could be made:

'{getFirstName} {lastName} did the thing.'.format(obj)
// -> 'Alec Baldwin did the thing.'

The above will continue to work the same way, but this PR additionally allows for passing arguments to these functions:

'{getFirstName true} {lastName} did the thing.'.format(obj)

// -> 'Billy Baldwin did the thing.'

Note, however, that even though I'm passing true the arguments passed are always strings. The function's if only checks for a truthy value in this simple case but if you were expecting anything else you'd have to cast it yourself ( eg. using Boolean or Number constructors, or parseInt() ).

Given this is a string formatting library, this limitation doesn't seem like much of a limitation.

Add the ability to pass arguments to object methods in a space delimited format. From what I understand, this would preclude strict adherence to Python's string formatting where spaces would be valid in keys when resolving against the object.
@haltcase
Copy link
Author

I'm seeing the failure on one of the tests and will be looking into that.

  1) format invokes methods:
     TypeError: String.prototype.toLowerCase called on null or undefined
      at toLowerCase (native)
      at lookup (index.js:9:3699)
      at index.js:9:2366
      at String.replace (native)
      at index.js:9:1108
      at Context.<anonymous> (test/index.js:84:8)

@davidchambers
Copy link
Owner

Thanks for the pull request. I would, though, like to keep the API as close as possible to Python's.

It's worth noting that the results you desire are attainable with the current API:

> '{1} {lastName} did the thing.'.format(obj, obj.getFirstName(false))
'Alec Baldwin did the thing.'

> '{1} {lastName} did the thing.'.format(obj, obj.getFirstName(true))
'Billy Baldwin did the thing.'

Change from a `null` binding to binding the method to its object. This fixes things like prototype methods (toLowerCase).
@haltcase
Copy link
Author

haltcase commented Nov 14, 2016

@davidchambers Not a problem, I assumed that would be the case. I can close out the PR if you want and maintain my own separate fork.

For my uses ( in a chat bot ), the template string is going to be set beforehand by end-users knowing they'll use things like {target}, {sender} etc. which will be replaced later. These are simple, but when adding something like {readfile ./path/to/file.txt} I think I need functionality like this PR allows.

edit: I'll ignore the lint issue then, but that's the only thing making it fail now. Ternary line!

@davidchambers
Copy link
Owner

I can close out the PR if you want and maintain my own separate fork.

It's a nice patch, but I think a fork is a good idea. One day I hope to complete and merge #2, so it's important not to drift from the Python API.

@haltcase
Copy link
Author

👍 sounds good.

@haltcase haltcase closed this Nov 14, 2016
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.

2 participants