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

Edit the docs to emphasize ShellStrings and Pipes #398

Closed
nfischer opened this issue Mar 17, 2016 · 7 comments
Closed

Edit the docs to emphasize ShellStrings and Pipes #398

nfischer opened this issue Mar 17, 2016 · 7 comments

Comments

@nfischer
Copy link
Member

The introduction of ShellStrings is a pretty big change, since a lot of people rely on the old behavior of strings have the .to() method (here's an example from our friends over at dthree/cash).

We might want to consider adding that back in to the String.prototype for now, and give a deprecation warning, since this might be a big impediment to users migrating to the new version. Or, if we decide to keep it as-is, we should document it a bit more clearly, and make it obvious for how people can create a file with the syntax:

ShellString('some text').to('file.txt');

Also, I think pipes are one of the more useful features in ShellJS v0.7, so these should probably be advertised better in the docs.

On a related note, it might be worth starting guides in the wiki pages about how to migrate from version to version. We've been introducing quite a few breaking changes, so it might ease the transition for users to have these breaking changes highlighted (and not only buried in the changelog with all the features/bug fixes/etc.).

@dthree
Copy link
Contributor

dthree commented Mar 17, 2016

Agreed. It's kind of awkward that ShellJS is not v1.0, when it has been such a stable part of the Node ecosystem for so long and is in production use in a ton of applications. It is treated by the community as a 1.0 already and should be 1.0, which would address these semver breaks.

@nfischer
Copy link
Member Author

Yeah, it's been fairly stable for a long time. Unfortunately, I think the project has had some big issues that really should be dealt with before we go to v1.0 (like how v0.6 and earlier modify the string prototype, with no way to opt-out).

In regards to this feature specifically, I'm leaning more toward adding it back into master as a default, and figuring out some way to opt-out (since the option to opt-out has been suggested as a suitable work-around). This would give people all the convenience they have now (and ShellJS is primarily about convenience, IMO), while also offering a way to make it comply more nicely with other modules. Any suggestions from the community would be greatly appreciated.

@dthree
Copy link
Contributor

dthree commented Mar 17, 2016

Okay nice. Makes sense.

When I was first playing with SHJS, I was a bit surprised on the String extension, but I figured there were just two sides to the coin. If you're using a global module, you have to expect that.

I consider global commands (rm, etc.) just as "bad" as extending String, honestly, so if I didn't want any of that in a project, I wouldn't import global.

@ariporad
Copy link
Contributor

@nfischer: what about if in global mode, normal strings have all the methods of ShellStrings? Then we can also add a 'noConflict' method.
On Thu, Mar 17, 2016 at 3:02 PM, dc [email protected] wrote:
Okay nice. Makes sense.

When I was first playing with SHJS, I was a bit surprised on the String extension, but I figured there were just two sides to the coin. If you're using a global module, you have to expect that.

I consider global commands ( rm , etc.) just as "bad" as extending String, honestly, so if I didn't want any of that in a project, I wouldn't import global.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub [https://github.com//issues/398#issuecomment-198102397]

@nfischer
Copy link
Member Author

I consider global commands (rm, etc.) just as "bad" as extending String, honestly, so if I didn't want any of that in a project, I wouldn't import global.

@dthree I agree with the reasoning there. I was considering suggesting exactly that. This provides convenience for those who want it, but keeps the pollution at 0 for when that's preferred. This would actually be fairly easy to implement, as well.

Then we can also add a 'noConflict' method.

@ariporad What do you mean by a 'noConflict' method?

The idea I'm leaning toward right now is:

  • Always have individual commands return ShellStrings, not regular JS strings
  • If using require('shelljs'), do not extend String.prototype
  • If using require('shelljs/global'), allow any JS string to have the .to() and .toEnd() methods only (not the other pipe methods). These methods are really ideally used after some other command anyway (like grep('o', 'file.txt').sed(/o/g, 'a')). This stays backwards compatible for that kind of require, which is the real motivation.

Thoughts?

@ariporad
Copy link
Contributor

SGTM

Ari

On Thu, Mar 17, 2016 at 6:06 PM, Nate Fischer [email protected]
wrote:

I consider global commands (rm, etc.) just as "bad" as extending String,
honestly, so if I didn't want any of that in a project, I wouldn't import
global.

@dthree https://github.com/dthree I agree with the reasoning there. I
was considering suggesting exactly that. This provides convenience for
those who want it, but keeps the pollution at 0 for when that's preferred.
This would actually be fairly easy to implement, as well.

Then we can also add a 'noConflict' method.

@ariporad https://github.com/ariporad What do you mean by a
'noConflict' method?

The idea I'm leaning toward right now is:

  • Always have individual commands return ShellStrings, not regular JS
    strings
  • If using require('shelljs'), do not extend String.prototype
  • If using require('shelljs/global'), allow any JS string to have the
    .to() and .toEnd() methods only (not the other pipe methods). These
    methods are really ideally used after some other command anyway (like grep('o',
    'file.txt').sed(/o/g, 'a')). This stays backwards compatible for that
    kind of require, which is the real motivation.

Thoughts?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#398 (comment)

@nfischer
Copy link
Member Author

Everything discussed in this thread has been addressed, so I'm closing this. Please reopen if you have any issues with the current state of the docs (otherwise, these will be the official docs for v0.7 until the next release).

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

No branches or pull requests

3 participants