HabitRPG / habitica

A habit tracker app which treats your goals like a Role Playing Game.
https://habitica.com
Other
11.98k stars 4.09k forks source link

Remove unused I18N strings #9957

Closed SabreCat closed 4 years ago

SabreCat commented 6 years ago

Description

As reported by @paglias in our internal bug tracking doc:

lots of unused / duplicate i18n strings. Should write an automated scripti for this.

Especially after the site redesign, there are many I18N strings that are not actually used. We should hunt these down and remove them to clean up our repo and reduce the burden on our Transifex volunteers.

Related: #9210

Progress

Alys commented 6 years ago

Copying viirus's comment from https://github.com/HabitRPG/habitica/issues/9210#issuecomment-362802915 :

"The apps don‘t use any of the strings from this repo directly. They only use the content and response strings from the api"

Therefore when searching for where strings are used, we need to search only the Habit.RPG/habitica repo

Alys commented 6 years ago

In https://github.com/HabitRPG/habitica/issues/9210#issuecomment-354697812, @Inshabel said they'd like to work on this so I've marked it as in progress for them.

Alys commented 6 years ago

A comment from @SabreCat in https://github.com/HabitRPG/habitica/issues/9210 :

"We should remove duplicate strings used for the same meaning. E.g. https://github.com/HabitRPG/habitica/pull/9191/commits/1717c2a5df3e649764d224eeee780f7b07d0cb24 - removed headGear because we had headgearCapitalized already. Word of caution! There are places where the same English text is used but it's used for different meanings of the word, and thus can be translated differently. E.g. text ("Title", meaning the heading text for a task) vs. title ("Title" meaning someone's contributor label)."

While that's still desirable, it wouldn't have to be done in this issue, and if there's any uncertainty about whether two identical strings might both be needed, it would be best to leave them both in place and we could investigate further later (e.g., ask the translators).

Alys commented 6 years ago

I've increased the priority on this because as noted in https://github.com/HabitRPG/habitica/issues/10008#issuecomment-367126096 having excess strings is an accessibility issue that slows down the website.

ghost commented 6 years ago

Hey, just an update on my progress.

Has been a bit slow due to the amount of strings to check and I've just started a new job in the last few weeks. Still working on this though and hope to have something ready to go soon.

Cheers

beffymaroo commented 6 years ago

@Inshabel Are you still working on this issue? Please let us know. Thank you!

ghost commented 6 years ago

Hi beffymaroo, I am still working on this. I've identified roughly 20,000 unused strings, Including duplicates over all languages (So about 710 per language).

I've removed about 10,000 so far, but it is a fairly long manual process, so is taking a lot longer than I'd initially expected.

Alys commented 6 years ago

@Inshabel I'm afraid you don't need to edit the other languages at all - only the files in the locales/en/ directory. The other languages will pick up all changes from Transifex. See the README files in those directories for more details.

If you've made changes to the files in the en directory, would you like to submit just those in a pull request?

ghost commented 6 years ago

Oh I see, sorry. Wasn't aware.

That should speed up the rest of the process then. I can submit a pull request today for that.

Thanks for the info, thought it was a longer process than it needed to be :/

beffymaroo commented 6 years ago

@paglias I see the PR linked above is closed- any label/status changes needed for this issue?

paglias commented 6 years ago

@beffymaroo no, the PR has been closed but not merged, a new one has been opened

SabreCat commented 6 years ago

I don't see an active PR (and whatever was mentioned on 16 Jun was never linked)... moving this back to help-wanted.

junewerner commented 4 years ago

I can grab this issue if it still needs doing. This feels like something I can bash-fu

Alys commented 4 years ago

@junewerner Thank you! I've marked this as in progress for you.

shanaqui commented 4 years ago

@junewerner Hi! Are you still interested in working on this issue? If not, or if we don't hear from you within a week, I'll mark it as help wanted again. No rush, just making sure it doesn't go off the radar. :)

akshay1992kalbhor commented 4 years ago

Hi, I found two keys with the same value in backgrounds.json:

{ "backgroundShop" : "Background Shop", "backgroundShopText": "Background Shop" }

I am assuming one of the above (key, value) pairs should be removed?

Alys commented 4 years ago

@akshay1992kalbhor Not necessarily, it's possible that both are used, and if so we may want to keep both. Even though they're both the same now, at some future time they may be given different values.

You can find out where they are used (if they are used) with git grep backgroundShop The results will contain a lot of hits from the translated locales files so if you want to remove them you can do git grep backgroundShop | grep -v /locales/

If the git grep command shows that one or both of the strings are not used, then they can be removed.

shanaqui commented 4 years ago

@junewerner Hi! Since I didn't hear back from you I'm marking this as help wanted again, but if you want to pick it back up in future, you can just let us know again!

RaitheOfDureya commented 4 years ago

Hello @shanaqui, if the issue #9957 is still open, I would like to give it a try. Cheers

paglias commented 4 years ago

Hi @RaitheOfDureya , the issue is still open, I'm marking it as in progress for you

RaitheOfDureya commented 4 years ago

Thanks, @paglias :slightly_smiling_face:

In a few minutes I will open the first Pull Request for this issue. To facilitate the review I decided to separate the PRs by json file within the locales folder. For each string (or blocks of strings) removed I made a commit, so that the proposed changes can be reversed if necessary without major impacts.

paglias commented 4 years ago

@RaitheOfDureya I've added a list in the issue description to keep track of the progress here

RaitheOfDureya commented 4 years ago

@paglias, I have already checked the testing.json and spells.json files. All strings are currently being used there, so you can also cross these two off the list.

RaitheOfDureya commented 4 years ago

@paglias, the questsContent.json can also be cross off the list, no unused strings there.

RaitheOfDureya commented 4 years ago

@paglias, I believe that the following strings are dynamically generated:

and used in website/client/src/components/shared/inventoryDrawer.vue and website/client/src/components/shops/market/index.vue:

... , { type: this.$t(`${type}ItemType`) });...

Could you confirm that for me? If so, you can cross inventory.json off the list.

paglias commented 4 years ago

@RaitheOfDureya those are indeed used, crossing off inventory.json

RaitheOfDureya commented 4 years ago

@paglias, the files front.json have only {} inside it. Should I remove them as I did with the maintenance.json?

paglias commented 4 years ago

@RaitheOfDureya front.json has some strings, see https://github.com/HabitRPG/habitica/blob/develop/website/common/locales/en/front.json

paglias commented 4 years ago

or do you meant that no strings are used? In that case yeah I'd remove the file but let's only do the english one for now, thanks!

RaitheOfDureya commented 4 years ago

@paglias, never mind. I was with the front.json file in locales\el open, instead of the one in locales\en.

RaitheOfDureya commented 4 years ago

@paglias, you can also cross death.json off the list. No unused strings there.

shanaqui commented 4 years ago

It looks like some necessary strings have gone missing! citrusella spotted that they had been mentioned.

One is a category for guilds called "organization" (some guilds have that category and now showing an error about a missing string), and the other one is to do with cancelling Apple subscriptions:

Hi guys, just noticed a small bug in the subscription page. At the bottom in the unsubscribe section immediately under "Cancel your Subscription?" I think there's supposed to be a string of text but instead mine just reads "String 'cancelSubInfoApple' not found." I'm on the desktop version using a Mac and Chrome!

Can we rule out whether these issues are caused by one of these PRs?

RaitheOfDureya commented 4 years ago

Thanks for reporting this issue, @shanaqui Unfortunately we cannot rule that out. This error was in fact caused by the removal of this string. Looking in detail and redoing the searches I found that these keys were passed dynamically and could be found using the regex \$t\((?!'). I must have overlooked, sorry for that.

in: website/client/src/components/settings/subscription.vue

getCancelSubInfo () {
      let payMethod = this.user.purchased.plan.paymentMethod || '';
      if (payMethod === 'Group Plan') payMethod = 'GroupPlan';
      return this.$t(`cancelSubInfo${payMethod}`);

I will add this comment on the corresponding PR.

paglias commented 4 years ago

@shanaqui @RaitheOfDureya I've added back the missing strings https://github.com/HabitRPG/habitica/commit/82e6d544c8dc9a35bada7c1618d3cea371b23459

I think the organization one was removed because it's an old category that can't be added anymore but was used by some categories and so it can only be found in the database but not in the code.

The fix will go live later today

citrusella commented 4 years ago

Re: "Organization" (maybe this isn't a good place but I don't know of a better one yet): Is that equivalent to the currently-available "Getting Organized" (getting_organized)? Would it make sense to find guilds using "organization" and change it out for "Getting Organized"? Organization-category guilds can't be filtered on the search and things with that tag don't show up when someone is trying to filter on "Getting Organized" (where it seems like they might want to show up)?

paglias commented 4 years ago

that's a good point @citrusella , do you mind opening an issue about that? thanks

citrusella commented 4 years ago

Done!

RaitheOfDureya commented 4 years ago

Discussion regarding the gear.json file

I spent a few hours analyzing this (tricky) file and its (countless) strings (>2000) . Using regular expressions I could find many strings being generated dynamically like the sets armorSpecial, headSpecial, shieldSpecial, weaponSpecial...

e.g.,

const textString = `armorSpecial${upperFirst(event)}${upperFirst(classNameString)}`;
...
      text: t(`${textString}Text`),
      notes: t(`${textString}Notes`, armorStats[klass]),
...

But some patterns I could only find searching manually. As for example the strings:

That I believe are being referenced in in website/common/script/content/constants/seasonalSets.js:

...
  spring: [
    // spring 2014
    'mightyBunnySet',
    'magicMouseSet',
    'lovingPupSet',
    'stealthyKittySet',
...

and website/common/script/content/gear/sets/special/index.js:

...
springRogue: {
  set: 'stealthyKittySet',
  canBuy: () => CURRENT_SEASON === 'spring',
},
...

From 2015 on, all other strings follow the guidelines suggested in website/common/locals/en/gear_README.md.

As this file is well organized and there are probably more patterns than due to the size we will probably overlook I suggest not to remove any string from it.

What is your opinion on this, @paglias ?

paglias commented 4 years ago

I agree to leave the gear.json file untouched

RaitheOfDureya commented 4 years ago

@paglias the backgrounds.json files are well maintained. You can cross it off the list since there is no unused strings there.

paglias commented 4 years ago

@RaitheOfDureya 🎉 thanks for all your work on this issue, I've merged the final PRs and noted them on your profile

RaitheOfDureya commented 4 years ago

I am happy to contribute :smile: