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

Deprecate {@if} and {@idx} #102

Merged
merged 1 commit into from
Nov 20, 2014
Merged

Conversation

sethkinast
Copy link
Contributor

Generates a WARN when these helpers are used. This would go into helpers 1.4.x, and these two helpers would be removed in 1.5.x.

@smfoote
Copy link
Contributor

smfoote commented Nov 12, 2014

The code looks good, but how will a dev know what to use instead of these deprecated helpers? Is there anyway we could add a link inside the message?

@sethkinast
Copy link
Contributor Author

That's a great call.

I'll build a wiki page and have _deprecated link to it.

@rragan
Copy link
Contributor

rragan commented Nov 12, 2014

1.4.0 is already out there and in use. Shouldn't the deprecation be after 1.5.x based on two releases of warning (since .1.4.x is already out there without it)

@rragan
Copy link
Contributor

rragan commented Nov 12, 2014

Less sure about @idx. People have proposed a parameter that would support a base value other than 0 which is really useful for some applications.

@sethkinast
Copy link
Contributor Author

[DUST WARN]: Deprecation warning: {@idx} is deprecated and will be removed in the next minor version of dustjs-helpers
[DUST WARN]: For help, see https://github.com/linkedin/dustjs-helpers/wiki/Deprecated-Features#idx

@jimmyhchan
Copy link
Contributor

Linking to #95

@sethkinast
Copy link
Contributor Author

My thought on an idx that doesn't start from 0 is just to use {@math} to do what you need; since it's now a variable instead of a helper it can be used as a param.

@sethkinast
Copy link
Contributor Author

@jimmyhchan
Copy link
Contributor

See #80 for an option to have @idx start with an offset

👎 we should probaly keep idx

@sethkinast
Copy link
Contributor Author

I don't super see the benefit of {@idx offset="4"/} vs {@math key="{$idx}" method="add" operand="4"/} but I am not nearly invested enough to mind not deprecating it 😛

Mostly it's just the Python principle of "there should be only one-- preferably obvious-- way to do it".

So take out the deprecation of {@idx} ?

Re: deprecation version, should a feature have to be deprecated in x.x.0 to be removed in x.x+1.0? I think that might be too slow, but I'm not hard-set.

@sethkinast
Copy link
Contributor Author

No, you're right-- because we need to have a feature available for the entire life of any version of dust supported by that version of the helpers.

So since 1.4.x supports up to Dust 2.5, we can't remove the feature until 1.6 which supports Dust 2.6 and 2.7.

I think I'll add a target param to _deprecate.

@rragan
Copy link
Contributor

rragan commented Nov 12, 2014

Yeh, the @ idx way is just more concise but I could live with @ math only way of doing it. As they are both helpers, they have the same general limitations as to where they can be used.

@sethkinast sethkinast force-pushed the deprecate branch 2 times, most recently from b1e3c28 to 43ef65a Compare November 12, 2014 23:45
prashn64 added a commit that referenced this pull request Nov 20, 2014
@prashn64 prashn64 merged commit c4681a6 into LinkedInAttic:master Nov 20, 2014
@sethkinast sethkinast deleted the deprecate branch December 11, 2014 08:13
@jimmyhchan jimmyhchan mentioned this pull request Jan 15, 2015
@myndzi
Copy link

myndzi commented Jan 21, 2018

I know this is way late, but I wanted to let you know of a combination of bad things caused by this PR :(

At least, with the version of dust that was active at this time, the log level defaulted to NONE, so the deprecation warning wasn't shown by default. Then, when the package was updated to a version after @if had been removed, it failed to function silently. Finally, this change happened with a minor version update, which means npm install will simply break a site when it shouldn't, with no warning (by default) before and no useful error afterwards.

This was a really bad experience, and I hope the procedure is different now and in the future :\

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.

6 participants