@sgiehl opened this Pull Request on June 12th 2015 Member

piwik/plugin-CustomAlerts#16 needs to be merged afterwards

@tsteur commented on June 14th 2015 Owner

I can imagine those strings might be used by a couple of other plugins? Especially Piwik PRO plugins but also other plugins maybe?

Would it make maybe sense to deprecate those keys first before completely removing them? Might be overdoing it, just a question. Would be otherwise at least worth mentioning in the developer changelog.

Only did a check for three strings. General_Seconds is eg also used in Piwik-API repository. CoreHome_PeriodWeek is eg used in Piwik Mobile, Piwik-API, UserCountryMap_Minutes is used in the Piwik OpenStreetMap plugin, ... so at least a mention in the changelog would be appropriate maybe? Not sure if we would have to list all changes or just link to this PR or so?

@mnapoli commented on June 15th 2015 Member

The move looks good.

For the question of backward compatibility it seems there's a piece of code to handle that in Translator (see the top of the diff), so that looks good to me. I'm +1 with that but we need to remove that code in 3.0, it would be good to create an issue about it maybe? (and mention v3.0 in the <a class='mention' href='https://github.com/todo'>@todo</a> maybe that way if we stumble on it later we know we can remove it)

@sgiehl commented on June 15th 2015 Member

I've updated the comment in Translator class and added a note to the changelog

@sgiehl commented on June 15th 2015 Member

@tsteur regarding the translations used in Piwik mobile. I've got an idea how to handle that a better way, but I'll create a ticket therefor. See piwik/piwik-mobile-2#5343

@tsteur commented on June 15th 2015 Owner

Just wondering: Is it easily possible to automatically generate a list of added and removed translation keys for a certain release? Could be nice for the future

@sgiehl commented on June 15th 2015 Member

Should not be that difficult. I'll have a look at that...

@tsteur commented on June 15th 2015 Owner

I'm just wondering where to list them then. Wouldn't want to bloat the changelog with them :)

@mattab commented on June 17th 2015 Owner

@sgiehl FYI in an attempt to clarify, created: https://github.com/piwik/piwik/issues/8125#issue-88896072

I suggested Translation keys, especially generic ones such as General_* and CoreHome_* keys, are part of the API and should not change. as suggestion. What do you think?

@mattab commented on June 18th 2015 Owner

added to 2.14.0 milestone

@sgiehl commented on June 18th 2015 Member

Making some of the translations a "part of the API" sound good. But we should consider which keys to make "permanent", so we do not have to change them frequently.

@tsteur commented on June 18th 2015 Owner

I reckon it's good to move more and more translation keys to an Intl repository so as long as this one follows Semver we won't break anything anyway (most likely g). So might be good to move even more into this repo before 3.0 and then we can just say all those starting with Intl_ are considered API.

For the Piwik specific keys like "Websites", "Super User", ... we could maybe use a different category like Piwik_.... Piwik_ and API_ would be not really good naming I reckon since we also have classes for that and it might be confusing to see it in code. Maybe there would be a better name? Or otherwise we use General_ and move the ones from General_ that are not considered API somewhere else.

This way it would be very clear for core developers and plugin developers when a translation key becomes API and which keys won't change etc. Eg we could just write "All translation keys that start with Intl or General are considered API, all other ones can change over time even though it is rather unlikely". Or something similar...

@sgiehl commented on June 20th 2015 Member

I agree with @tsteur
Guess we should create a new ticket for that. Also we maybe should afterwards adjust the translation search in piwik to show which keys are "static" and which might change...

@mattab commented on June 22nd 2015 Owner

Hi @sgiehl sounds good. Do you mind create follow up issue, and merge the PR?

This Pull Request was closed on June 22nd 2015
Powered by GitHub Issue Mirror