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

removes CLI #53

Merged
merged 4 commits into from
Nov 25, 2016
Merged

removes CLI #53

merged 4 commits into from
Nov 25, 2016

Conversation

jonschlinkert
Copy link
Collaborator

Related to #36.

Thanks! (and sorry for taking so long)

@phated
Copy link

phated commented Jul 13, 2016

As per #36 (comment) - ping @felixge

@jonschlinkert
Copy link
Collaborator Author

thanks @phated, good memory!

@felixge
Copy link
Owner

felixge commented Jul 16, 2016

This breaks bin/cli.js. Please fix it or remove it as well.

@jonschlinkert
Copy link
Collaborator Author

No problem, I thought that was removed. will do it now

@felixge
Copy link
Owner

felixge commented Jul 29, 2016

@jonschlinkert LGTM. What's your npm user name? I'll add you as a repo contributor and npm owner so you can publish this change yourself.

@jonschlinkert
Copy link
Collaborator Author

great, thanks! it's the same, jonschlinkert

@jonschlinkert
Copy link
Collaborator Author

@felixge ping, just a reminder to add me to npm, thx!

@felixge
Copy link
Owner

felixge commented Aug 30, 2016

@jonschlinkert done. sorry for delay

@jonschlinkert
Copy link
Collaborator Author

thanks! much appreciated.

@MarkHerhold
Copy link

@jonschlinkert Did you manage to get the access you need for this?

@jonschlinkert
Copy link
Collaborator Author

@MarkHerhold yes. however, I'd like some feedback before I do this.

The main issue I'm hoping to resolve is that every install of gulp comes with ~48 extra deps that don't do anything at all but take up space. This is not the end of the world by any means, but removing these could make installations that much faster.

To get to the point, if I publish this as a major (which I would normally do), the problem I'm hoping to solve won't be solved - unless we can bump this in gulp-util as well, which is locked afaik.

@phated would you be willing to make an exception and bump this one module in gulp-util? Or do you have a suggestion on how we can do this so that dateformat gets a proper bump, and we also remove these extra deps from gulp-util?

@phated
Copy link

phated commented Sep 9, 2016

Yeah, I can bump it.

@tunnckoCore
Copy link

tunnckoCore commented Sep 9, 2016

Yea, let's create datetime-cli for that thing?

@jonschlinkert
Copy link
Collaborator Author

awesome! I'll do this tonight. @phated want me to do a pr to bump, or do you want to do it? thanks, I really appreciate it.

@jonschlinkert
Copy link
Collaborator Author

@felixge looks like I don't have write access to the repo, so I can't merge.

@MarkHerhold
Copy link

@jonschlinkert Do we also want to squeeze in #40? Just being opportunistic... ;)

@jonschlinkert
Copy link
Collaborator Author

@MarkHerhold that makes sense. If we're going to do a major, that looks like low-hanging fruit. Although I should quell any more opportunism by saying that I won't have much more time to review other prs atm.

@MarkHerhold
Copy link

@jonschlinkert fine by me!

@phated
Copy link

phated commented Sep 13, 2016

@jonschlinkert feel free to submit a PR on gulp-util

@jonschlinkert
Copy link
Collaborator Author

no prob, once we get this resolved I'll create a pr, thx

@alexindigo
Copy link

@jonschlinkert Just curious where things stand. Thanks.

@jonschlinkert
Copy link
Collaborator Author

looks like I don't have write access to the repo, so I can't merge.

@felixge ping

@felixge
Copy link
Owner

felixge commented Nov 8, 2016

@jonschlinkert added you. If you give me your NPM user handle, I'll add you there as well.

@jonschlinkert
Copy link
Collaborator Author

jonschlinkert commented Nov 9, 2016

thanks! my username is jonschlinkert on npm

edit: actually you already added me to npm, just double-checked. thanks again!

@felixge
Copy link
Owner

felixge commented Nov 9, 2016

@jonschlinkert added you on npm as well.

@doowb
Copy link
Collaborator

doowb commented Nov 9, 2016

if I publish this as a major (which I would normally do)

Since the cli was added as a patch bump, I think this should just be a patch bump. Also, the module's api hasn't changed (which is the main use case). I think it'll save other people's time too, since they won't have to explicitly update anything.

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.

7 participants