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

New Release #267

Closed
svozza opened this issue Apr 9, 2015 · 22 comments
Closed

New Release #267

svozza opened this issue Apr 9, 2015 · 22 comments
Milestone

Comments

@svozza
Copy link
Collaborator

svozza commented Apr 9, 2015

There's been a lot of new features that have been added since the last release, it might be best to pull the trigger on this sooner rather than later. Here's a list of all the changes, I think we should copy and paste it into the release notes in the Releases section.

New additions

Improvements

@vqvu
Copy link
Collaborator

vqvu commented Apr 9, 2015

+1 on new release.

One more improvement

@svozza
Copy link
Collaborator Author

svozza commented Apr 9, 2015

Ah I'd missed that one. Incidentally, if we're finally bringing in drop should we also merge slice (#250)?

@jeromew
Copy link
Collaborator

jeromew commented Apr 9, 2015

I tried to make a release but eslint is not happy when calling npm test

lib/index.js
2896:40 error err is already declared in the upper scope no-shadow
3281:29 error memo is already declared in the upper scope no-shadow
3291:27 error memo is already declared in the upper scope no-shadow
3987:18 error Empty block statement no-empty

✖ 4 problems (4 errors, 0 warnings)

do you have the same issue on your side ? it is probably due to recent commits.

@vqvu
Copy link
Collaborator

vqvu commented Apr 9, 2015

Slice has optional arguments. Not sure what to do about that...

@jeromew, that's strange. I don't see it. Fresh clone and everything.

On Thu, Apr 9, 2015 at 3:22 PM, jeromew notifications@github.com wrote:

I tried to make a release but eslint is not happy when calling npm test

lib/index.js
2896:40 error err is already declared in the upper scope no-shadow
3281:29 error memo is already declared in the upper scope no-shadow
3291:27 error memo is already declared in the upper scope no-shadow
3987:18 error Empty block statement no-empty

✖ 4 problems (4 errors, 0 warnings)

do you have the same issue on your side ? it is probably due to recent
commits.


Reply to this email directly or view it on GitHub
#267 (comment).

@apaleslimghost
Copy link
Collaborator

It's possible to configure Travis to release to npm when a tagged commit passes. Do we want to consider this so we don't have to wait for @caolan or @jeromew to release?

@vqvu
Copy link
Collaborator

vqvu commented Apr 10, 2015

Wouldn't this expose their NPM credentials to the public? Unless travis encrypt uses a repository-specific public key.

On Fri, Apr 10, 2015 at 1:52 PM, Matt Brennan notifications@github.com
wrote:

It's possible to configure Travis to release to npm when a tagged commit
passes http://docs.travis-ci.com/user/deployment/npm/. Do we want to
consider this so we don't have to wait for @caolan
https://github.com/caolan or @jeromew https://github.com/jeromew to
release?


Reply to this email directly or view it on GitHub
#267 (comment).

@svozza
Copy link
Collaborator Author

svozza commented Apr 13, 2015

Regarding slice, I had noticed that although it's worth noting that it actually doesn't affect the currying. These all work as expected:

var arr = [1, 2, 3, 4, 5];

var first = _.slice(1);
var second = _.slice(1, 4);

first(3, _(arr)).toArray(_.log);
//=> [2, 3]

second(_(arr)).toArray(_.log);
//=> [2, 3, 4]

I prefer fixed arity functions though so a solution would be to just remove the default values in slice and be explicit in the implementation of drop:

Stream.prototype.drop = function (n) {
    if (n <= 0) {
        return this;
    }
    return this.slice(n, Infinity);
};
exposeMethod('drop');

@vqvu
Copy link
Collaborator

vqvu commented Apr 13, 2015

The problem with optional arguments has always been if people try to do

var drop5 = _.slice(5);
drop5(_([1, 2, 3, 4, 5])).toArray(_.log);
// => an error.

Not having optional args means people can't accidentally mess up like this. But, I'm also OK with relaxing our stance on optional args if we put a warning in big red letters in the docs that says the top-level slice requires an explicit end parameter. We'll just be pushing the hard work to the user.

@svozza
Copy link
Collaborator Author

svozza commented Apr 13, 2015

I think I would rather keep things consistent and just have the blanket rule that transforms don't take optional arguments, they complicate the API and once that genie is out of the bottle it's hard put it back in.

@vqvu
Copy link
Collaborator

vqvu commented Apr 13, 2015

I don't feel strongly either way, but slice is so ubiquitous that not having end be optional could cause the same kinds of confusion. Or we could implement drop and take in terms of a slice-like function, but not expose a slice transform.

@svozza
Copy link
Collaborator Author

svozza commented Apr 16, 2015

Sorry, meant to reply to this. slice is quite useful though so it would be a shame to lose it. Does anyone else have any input? I'll go along with the consensus.

@vqvu
Copy link
Collaborator

vqvu commented Apr 16, 2015

We could go the reduce route and have a slice1.

On the other hand, slice(start, end) is equivalent to .drop(start).take(end - start).

@caolan
Copy link
Owner

caolan commented Apr 16, 2015

I'd vote for keeping fixed arity, I understand this would likely trip people up in the case of slice though. Any more suggestions on alternative names for slice? Perhaps 'substream(start, end)', similar to substr()?

@jeromew
Copy link
Collaborator

jeromew commented Apr 16, 2015

@vqvu ok ; after removing node_modules altogether + a fresh npm install, the linting issue has disappeared.

I still don't understand it because it looks like the

2896:40 error err is already declared in the upper scope no-shadow

issue really exists in the code line 2896 (there is an err variable used by the closure that shadows the err from the upper scope)

am i misunderstanding this 'no-shadow' directive ?

@jeromew
Copy link
Collaborator

jeromew commented Apr 16, 2015

@svozza you said

I think we should copy and paste it into the release notes in the Releases section.

I think we should use the CHANGELOG file instead. It hasn't been used since 2.0.0 but should probably be used from now on.

regarding other options for slice, there could be cut, extract, section. cut is the word used for video cutting for instance when you want to extract a slice of a video.

@svozza
Copy link
Collaborator Author

svozza commented Apr 16, 2015

Yeah, I just mentioned the Releases section because I saw it on the Bluebird page but changelog is as good a place as any. If we change the file name to changelog.md we can get the markup to work in it as well.

@vqvu
Copy link
Collaborator

vqvu commented Apr 16, 2015

We'll want to fix #205 before we release or the page that gets generated will be broken again.

@jeromew Perhaps the no-shadow was turned off by default in a later version of eslint? We don't explicitly enable that check in our .eslintrc.

I like substream since cut could also mean split (at a first glance).

@vqvu
Copy link
Collaborator

vqvu commented Apr 17, 2015

Thought about it some more, and I now think it's just fine to have slice without the optional argument (so objection withdrawn! :)).

After all, if I was OK with slice and slice1, then I should be OK with slice and drop.

The PR is fine to merge, I think, after applying @svozza's proposed fix.

@vqvu vqvu added this to the v2.5.0 milestone Apr 17, 2015
@jeromew
Copy link
Collaborator

jeromew commented Apr 17, 2015

@vqvu so if I understand well you had the main objection regarding the slice name but that objection is gone now. @caolan was your message an objection also or do you agree with the slice name ?

@vqvu
Copy link
Collaborator

vqvu commented Apr 17, 2015

@jeromew Yes.

@svozza
Copy link
Collaborator Author

svozza commented Apr 21, 2015

I'd say everything for this release is merged now, we should depoly to NPM as soon as possible.

@jeromew
Copy link
Collaborator

jeromew commented May 4, 2015

Sorry everyone about the delay.
I updated the CHANGELOG, renamed it CHANGELOG.md, mentioned batchWithTimeOrCount that was merged in between and pushed 2.5.0 to npm.
thanks to everyone for this release !

@jeromew jeromew closed this as completed May 4, 2015
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

5 participants