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

prevent 'to: wrong argument' output caused by shelljs #128

Merged
merged 1 commit into from
Mar 26, 2015

Conversation

az7arul
Copy link
Collaborator

@az7arul az7arul commented Feb 6, 2015

`to` method in shelljs expects two arguments and throws an error if there is no second argument, so passing an extra string to keep it happy

This fixes #127 although I am not entirely sure if we should do it or not.

@jprichardson thoughts ?

@az7arul az7arul changed the title a hack to get rid of output thrown if shelljs is required before this prevent 'to: wrong argument' output caused by shelljs Feb 6, 2015
@jprichardson
Copy link
Owner

Thank you.

I'm definitely OK with accommodating shell.js and its bad behavior solely because it's so commonly used. I like this hack.... I'm wondering though, does it make more sense to just hardcode the native string properties? That adds more to the payload, but it feels more correct. IDK, maybe what you have is good enough. Thoughts?

@az7arul
Copy link
Collaborator Author

az7arul commented Feb 6, 2015

This hack does allow adding own String.prototype properties ( which seems to me a bad idea but it is done i.e shelljs) since the current implementation wraps them if they were added before requiring string.js.

Having said that, If adding hardcoded property does not break anything I think thats the way to go.

@steveoh
Copy link

steveoh commented Mar 26, 2015

is this going to get merged?

@jprichardson
Copy link
Owner

Yeah, I'll do it now... give me a few mins...

jprichardson added a commit that referenced this pull request Mar 26, 2015
prevent  'to: wrong argument' output caused by shelljs
@jprichardson jprichardson merged commit 666605a into jprichardson:master Mar 26, 2015
@jprichardson
Copy link
Owner

Published as 3.1.1.

@steveoh
Copy link

steveoh commented Mar 26, 2015

clap

@steveoh
Copy link

steveoh commented Apr 7, 2015

@jprichardson I'm noticing a side affect with this. When this is executed with the shelljs's overload of to a file is written named string with the value test. This is a bit crazy, but do we need to put more logic in here to tame shell js for their actions?

@jprichardson
Copy link
Owner

Grrrr on shelljs. Would skipping the String.prototype.to method maybe work? @az7arul thoughts?

@az7arul
Copy link
Collaborator Author

az7arul commented Apr 8, 2015

@jprichardson we can have a list of string prototype methods that we allow in stringjs instead of going through all String.prototype methods ( some of which can be non native ) and calling them with string arguments to determine if they return string or not.

This approach should get rid of these kind of problems but might be a breaking change for people who are relying on stringjs to recognize string methods that they have added themselves and allow chaining.

@az7arul
Copy link
Collaborator Author

az7arul commented Jun 3, 2015

we need to skip both to and toEnd

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.

to: wrong arguments
3 participants