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

remove unnecessary aliases #171

Closed
1 task done
jprichardson opened this issue Aug 10, 2015 · 15 comments
Closed
1 task done

remove unnecessary aliases #171

jprichardson opened this issue Aug 10, 2015 · 15 comments

Comments

@jprichardson
Copy link
Owner

Inspired by this: ramda/ramda#574 (comment) most aliases should be removed...

  • remove delete() alias to remove(). Most style guides frown upon using a reserved keyword for a field / method name. So this should definitely be removed.

Questions:

  1. Should ensureDir()/mkdirp()/mkdirs() be condensed into one?
  2. json methods... should it be Json or JSON? I'm inclined to keep both of these variations but keep promoting Json.
@jprichardson jprichardson added this to the 1.0 milestone Aug 10, 2015
@jprichardson jprichardson changed the title remove 'delete' alias remove unnecessary aliases Aug 10, 2015
@jprichardson
Copy link
Owner Author

Removed delete() and deleteSync():

955836b

motiazu added a commit to motiazu/gulp-directory-sync that referenced this issue Dec 13, 2015
…-extra update jprichardson/node-fs-extra#171

Change comments about deleting to removing to keep a common terminology in the code
@martinheidegger
Copy link

Just my 2cnt:

  1. mkdirp is best know - i'd reduce it to that (remove the others)
  2. Technically JSON is an acronym and as such should be upper-case (the JSON object is upper-case as well)

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 26, 2016

  1. Definitely remove mkdirs, IDK about ensureDir.

  2. I'm inclined to keep both of these variations but keep promoting Json.

    👍

@jprichardson
Copy link
Owner Author

Definitely remove mkdirs

I thought mkdirs was more newbie-friendly than mkdirp. Interesting. Of the three, I prefer to have only one way. ensureDir seems very clear to me... but I think it would confuse people not having mkdirp. IDK here.

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 27, 2016

LOL, I guess part of what makes me prefer mkdirp is the fact that I write quite a few npm scripts, so I tend to think in terms of bash commands.

Perhaps push this off to post-v1.0.0?

@jprichardson jprichardson removed this from the 1.0.0 milestone Oct 27, 2016
@RyanZim
Copy link
Collaborator

RyanZim commented Dec 26, 2016

@jprichardson Perhaps we can revisit this for v2? I really don't know what to think.

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 8, 2017

@manidlou Would like to know your opinions here.

@manidlou
Copy link
Collaborator

manidlou commented Apr 9, 2017

I am more inclined to keep mkdirp mainly because I use bash a lot; therefore I am more comfortable with mkdirp. Between JSON and Json, I go with Json. IDK, having a function name with all uppercase as the second word like readJSON looks a little peculiar to me! But it's just me! I am still open to both options though.

@ghost
Copy link

ghost commented Apr 30, 2017

readJSON would look a lot better, because JSON is an uppercase acronym. mkdirp seems best

@c-vetter
Copy link

c-vetter commented Jul 7, 2017

Personally, I'd prefer readJSON because, as said before, JSON is the proper capitalization, but use readJsonSync when I need to integrate with mandatorily synchronous code, because I find readJSONSync looks kind of weird. Generally, I'd go with JSON.

As for mkdirp et al., I'm personally not so fond of making everything bashy. However, seeing as the base fsis setup like that anyway, I'd stick with it. Therefore, mkdirp seems most appropriate for a package that seeks to be a seemless drop-in replacement.

@ghost
Copy link

ghost commented Jul 7, 2017

@rasenplanscher I would still go for readJSONSync, because I would assume that you should just be able to add a Sync onto the end without worrying about changing capitalisation.

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 5, 2018

@jprichardson @manidlou @JPeer264 given the amount of divergence in ideas here, and how much code in the wild would be affected, I vote we close this and keep the aliases as they stand for now, but make an effort not to add new aliases in the future.

@JPeer264
Copy link
Collaborator

JPeer264 commented Feb 5, 2018

I read through the comments here, it seems that everybody has different prefs. As this library has got those aliases over some time now, it might be a bit confusing if the one or other preferred alias is not available anymore in the next major release. I would also suggest to keep those aliases, but don't add any new aliases in the future.

I would just remove aliases if they don't make sense, but here, all aliases make sense for me.

@manidlou
Copy link
Collaborator

manidlou commented Feb 5, 2018

Agreed!

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 20, 2018

Closing for now as per #171 (comment); @jprichardson reopen if you disagree.

@RyanZim RyanZim closed this as completed Feb 20, 2018
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

No branches or pull requests

6 participants