Closed Bug 1013891 Opened 6 years ago Closed 6 years ago

Move pref and message-manager strings to constants

Categories

(Firefox :: Translation, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

Attachments

(1 file)

In Translation.jsm I noticed that pref names and message identifiers are hardcoded and duplicated inside methods.

Preferably, we'd move these strings to the top of each file to
1) improve readability
2) improve usability
3) prevent typos in the future.
Flags: firefox-backlog+
Marco, can you add this to the current iteration?
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
Whiteboard: p=2 [qa-]
Blocks: 1013861
Added to Iteration 32.2
Flags: needinfo?(mmucci)
Whiteboard: p=2 [qa-] → p=2 s=it-32c-31a-30b.2 [qa-]
Comment on attachment 8426891 [details] [diff] [review]
Patch v1: move often used strings to shared constants

Review of attachment 8426891 [details] [diff] [review]:
-----------------------------------------------------------------

I think the cost of hiding the constant values in a separate file and of having to use the preprocessor outweighs the benefit of reducing the typo risk.

I think I would prefer having the constants defined at the top of both file. That's a small duplication, but the risk of typo while copying whole lines seems almost non existent.

::: browser/components/translation/constants.js
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#endif
> +
> +const TRANSLATION_SOURCE_LANGUAGES = ["en", "zh", "ja", "es", "de", "fr", "ru", "ar", "ko", "pt"];
> +const TRANSLATION_TARGET_LANGUAGES = ["en", "pl", "tr", "vi"];

These 2 arrays weren't duplicated, right?
Attachment #8426891 - Flags: review?(florian) → review-
No longer blocks: 1013861
Depends on: 1013861
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> I think the cost of hiding the constant values in a separate file and of
> having to use the preprocessor outweighs the benefit of reducing the typo
> risk.
> 
> I think I would prefer having the constants defined at the top of both file.
> That's a small duplication, but the risk of typo while copying whole lines
> seems almost non existent.

Sure, matter of taste, but sure. I'll post an updated patch later.

NB: the reason I opted for this method is because I generally like to avoid having to browse through a list of constants before I get to the meat of a module. I consider it boilerplate and not volatile code (hence the term 'constant'). However, the cost of separating constants to their own respective module is too much work and, as we all know, module loading is not cheap. Hence the choice to include it using pre-processor directive, because that's free at run-time.
I actually don't like the indirection of using constants for this - the typo risk seems small (and should be covered by tests!), and it adds an extra step when grepping for where messages are sent/observed, which I find make understanding the code slightly harder.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Flags: firefox-backlog+ → firefox-backlog-
Whiteboard: p=2 s=it-32c-31a-30b.2 [qa-]
You need to log in before you can comment on or make changes to this bug.