Sort properties file and remove duplicates #10647

srirambv · 2017-08-24T09:01:18Z

Submitter Checklist:
Remove duplicate strings in properties file
Sort string alphabetically in all properties file
Merged error.properties into errorMessage.properties

Fixes : #3958

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:
Verify all text in browser is shown as before

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

luixxiul · 2017-08-24T11:18:28Z

I'm wondering if the strings should also be sorted semantically, separating lines with comment lines.

srirambv · 2017-08-28T08:30:31Z

@luixxiul Could you provide more details ?

luixxiul · 2017-08-28T13:30:28Z

app/extensions/brave/locales/en-US/adblock.properties

+customFilters=Custom filters
+disableAdBlockForSite=Disable Ad block
+lastCheckETagLabel=Last check ETag:
+lastUpdateCheckDateLabel=Last update check:


@srirambv I mean like this:

label_lastUpdateCheckDate=... label_lastCheckETag=...

Specifying a hierarchy with dashes or something more proper would make it easier for us to see the system of the localization strings and enhance it when new ones get added.

@luixxiul I'd suggest creating a new issue and proposing the strategy. If it's something that is easier for translators and also for programmers, then maybe we can adapt it. Personally, I don't know how helpful it would be

luixxiul · 2017-08-28T13:36:24Z

app/extensions/brave/locales/en-US/autofill.properties

+autofillData=Autofill data
+autofillTitle=Autofill Settings
+city=City
+country=Country


@srirambv another example:

addresses_city=... addresses_country=... ... creditCards_edit=... creditCards_expirationDate=...

The hierarchy has not been explicitly specified with a certain rule which we should follow when adding/editing the localization strings. Creating that rule would increase maintainability.

@luixxiul definitely something we'd want proposed in its own issue

bsclifton · 2017-08-28T16:27:35Z

app/extensions/brave/locales/en-US/error.properties

@@ -1,9 +0,0 @@
-errorReload=Retry the URL


Was removing this file intended? It's still referenced in a few places:

browser-laptop/app/locale.js

Line 384 in e691f76

path.join(__dirname, 'extensions', 'brave', 'locales', lang, 'error.properties'),

browser-laptop/app/extensions/brave/about-contributions.html

Line 19 in e691f76

<link rel="localization" href="locales/{locale}/error.properties">

browser-laptop/app/extensions/brave/about-error.html

Line 17 in e691f76

<link rel="localization" href="locales/{locale}/error.properties">

browser-laptop/app/extensions/brave/about-preferences.html

Line 19 in e691f76

<link rel="localization" href="locales/{locale}/error.properties">

(possibly others)

bsclifton

Comments left 😄 The issue mentioned is that error.properties is referenced a few places which will break if the file is deleted. We should remove those references also

Do you have a list of the duplicate strings? (just curious how many there are / where they were)

srirambv · 2017-08-29T08:53:58Z

@bsclifton removed the references for error.properties

Do you have a list of the duplicate strings? (just curious how many there are / where they were)

There weren't much may be about 10. Most were in autofill and bookmark

codecov-io · 2017-09-04T17:42:16Z

Codecov Report

Merging #10647 into master will increase coverage by 0.39%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10647      +/-   ##
==========================================
+ Coverage   54.16%   54.55%   +0.39%     
==========================================
  Files         247      244       -3     
  Lines       21543    21128     -415     
  Branches     3334     3262      -72     
==========================================
- Hits        11669    11527     -142     
+ Misses       9874     9601     -273

Flag	Coverage Δ
#unittest	`54.55% <ø> (+0.39%)`	⬆️

Impacted Files	Coverage Δ
app/browser/windows.js	`17.82% <0%> (-6.49%)`	⬇️
app/renderer/lib/extensionsUtil.js	`55.84% <0%> (-3.93%)`	⬇️
app/renderer/components/preferences/pluginsTab.js	`45.83% <0%> (-3.19%)`	⬇️
app/common/state/tabState.js	`77.02% <0%> (-2.29%)`	⬇️
app/browser/tabs.js	`26.64% <0%> (-2.24%)`	⬇️
js/settings.js	`96.92% <0%> (-1.54%)`	⬇️
js/actions/webviewActions.js	`9.25% <0%> (-1.38%)`	⬇️
js/stores/appStore.js	`12.45% <0%> (-1.06%)`	⬇️
app/renderer/reducers/frameReducer.js	`26.31% <0%> (-0.76%)`	⬇️
app/filtering.js	`18.07% <0%> (-0.51%)`	⬇️
... and 19 more

bsclifton

LGTM 😄

luixxiul · 2017-09-05T04:20:09Z

Also maybe related;

See deleteBookmark.

srirambv · 2017-09-05T04:25:29Z

This is what I see on master with this merged

bsclifton · 2017-09-05T04:45:27Z

there are definitely issues with this- I'm going to have to revert

bsclifton · 2017-09-05T04:51:00Z

reverted with ab60908

@srirambv since there is a large impact to the app, maybe if we tried this again we should only sort (and not remove dupes / consolidate files)

srirambv · 2017-09-05T04:53:40Z

Sure will only do a sort

srirambv added the l10n label Aug 24, 2017

srirambv requested review from aekeus, luixxiul and bsclifton August 24, 2017 09:01

NejcZdovc assigned srirambv Aug 25, 2017

luixxiul reviewed Aug 28, 2017

View reviewed changes

bsclifton reviewed Aug 28, 2017

View reviewed changes

bsclifton suggested changes Aug 28, 2017

View reviewed changes

bsclifton added priority/low QA/no-qa-needed release-notes/exclude labels Sep 4, 2017

srirambv added 2 commits September 4, 2017 10:41

Sort properties file and remove duplicates

404fef6

remove error.property references

b8fae14

bsclifton approved these changes Sep 4, 2017

View reviewed changes

bsclifton added this to the 0.21.x (Nightly Channel) milestone Sep 4, 2017

bsclifton merged commit 8993297 into brave:master Sep 4, 2017

bsclifton removed this from the 0.21.x (Nightly Channel) milestone Sep 5, 2017

bsclifton added the reverted label Sep 5, 2017

bsclifton removed QA/no-qa-needed release-notes/exclude labels Sep 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort properties file and remove duplicates #10647

Sort properties file and remove duplicates #10647

srirambv commented Aug 24, 2017

luixxiul commented Aug 24, 2017

srirambv commented Aug 28, 2017

luixxiul Aug 28, 2017

bsclifton Aug 28, 2017

luixxiul Aug 28, 2017 •

edited

Loading

bsclifton Aug 28, 2017

bsclifton Aug 28, 2017

bsclifton left a comment

srirambv commented Aug 29, 2017

codecov-io commented Sep 4, 2017

bsclifton left a comment

luixxiul commented Sep 5, 2017

srirambv commented Sep 5, 2017

bsclifton commented Sep 5, 2017

bsclifton commented Sep 5, 2017

srirambv commented Sep 5, 2017

Sort properties file and remove duplicates #10647

Sort properties file and remove duplicates #10647

Conversation

srirambv commented Aug 24, 2017

luixxiul commented Aug 24, 2017

srirambv commented Aug 28, 2017

luixxiul Aug 28, 2017

Choose a reason for hiding this comment

bsclifton Aug 28, 2017

Choose a reason for hiding this comment

luixxiul Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

bsclifton Aug 28, 2017

Choose a reason for hiding this comment

bsclifton Aug 28, 2017

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

srirambv commented Aug 29, 2017

codecov-io commented Sep 4, 2017

Codecov Report

bsclifton left a comment

Choose a reason for hiding this comment

luixxiul commented Sep 5, 2017

srirambv commented Sep 5, 2017

bsclifton commented Sep 5, 2017

bsclifton commented Sep 5, 2017

srirambv commented Sep 5, 2017

luixxiul Aug 28, 2017 •

edited

Loading