search

AmpersandJS / amp

a collection of individual JS utility modules
http://amp.ampersandjs.com
MIT License
130 stars 12 forks source link

Allow conditionals to be regular functions in toggle-class function #51

Closed kamilogorek closed 9 years ago

kamilogorek commented 9 years ago

This allows us to determine whether item should have modified class or not, based on it's own data.

aaronmccall commented 9 years ago

:+1: looks good to me

HenrikJoreteg commented 9 years ago

I think this makes sense, but should have a corresponding update to sig.md and doc.md files.

kamilogorek commented 9 years ago

@HenrikJoreteg docs updated, however I'm not sure how to mark parameter that can be in multiple forms, eg. boolean || function in sig.md.

HenrikJoreteg commented 9 years ago

@kamilogorek did this before I think that works ok https://github.com/AmpersandJS/amp/blob/master/modules/each/sig.js

HenrikJoreteg commented 9 years ago

also, @kamilogorek, set the new version to the package.json on how the change impacts this. It's nice to do it as part of the PR since there's sometimes some discussions around this.

HenrikJoreteg commented 9 years ago

In terms of versioning, this one is a bit tricky. One could argue that it's a major version bump because if a user was using a function as the truthy conditional, previously, this would potentially change the result.

Having said that, that's certainly pretty edge-casey and it certainly isn't seeing heavy use (see stats here: https://www.npmjs.com/package/amp-toggle-class) and no one has a published module that uses this as a dependency. We've talked about potentially saying that the tests are the only contract. Meaning, if you use it in a way not covered by the tests, then you're not guaranteed.

This is very similar to this one: https://github.com/AmpersandJS/amp/issues/32

So, i'm unsure.

adamyonk commented 9 years ago

I can see this warranting a bump because of the tests changing (and because of what was mentioned in #32).

HenrikJoreteg commented 9 years ago

If you look close, the only thing changing about any existing test is the description of the result, everything else is new tests being added. If we had to change actual existing tests I'd be 100% on major bump.

adamyonk commented 9 years ago

Ah, that makes sense. I take it back!

mike-engel commented 9 years ago

IMO, consistency is always better. If it’s a breaking change, major bump unless it’s a 0.x module.

HenrikJoreteg commented 9 years ago

@mike-engel so just to clarify, you're saying make it 2.0.0 right?

jdalton commented 9 years ago

I find if I'm hemming and hawing over whether it's a breaking change it's better to side with the major bump. If you don't want to bump now, you can always put it on your roadmap and implement it at a later time. This way your users won't get any surprises and they can trust your versioning. If you're holding off because of some attachment to v1.0, then the best thing to do is

Let it go!
mike-engel commented 9 years ago

@HenrikJoreteg Yeah. Sorry for the confusion :smile:

fyockm commented 9 years ago

@jdalton nice frozen reference.

I must say this seems like a minor bump to me. It's not changing the API, just clarifying some ambiguity.

HenrikJoreteg commented 9 years ago

I'm with @fyockm on this, it's adding functionality without requiring any tests to change to make it pass, sounds like a minor bump to me. I know the argument could be made for major, but it seems extremely unlikely this would break anything for anyone, especially at this point in the project.

HenrikJoreteg commented 9 years ago

published as 1.1.0, site re-built: http://amp.ampersandjs.com/#amp-toggle-class thanks all