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

i18n UI messages, and start showing explanations for rules from the old checker #160

Merged
merged 5 commits into from
Mar 13, 2015

Conversation

tripu
Copy link
Member

@tripu tripu commented Mar 10, 2015

Changes, in a nutshell (a large nutshell):

  • The same UI messages that were in l10n.js are now duplicated in two languages: l10n-en_GB.js and l10n-es_ES.js.
  • …which are identical right now (both are in English), except for the values of these two keys: heuristic.group.candidate and heuristic.group.not-found, which are translated into Spanish (for testing purposes).
  • Added a field lang to the general configuration in validator.js (set to en_GB).
  • Tweaked slightly @deniak's rules.json so that now, instead of being a huge array, it's a huge object. And the IDs of profiles (WD, REC, etc) are the keys in that object.
  • …and that is to be able to maintain a new file of selectors, l10n-selectors.js. This file associates each ID of a user message (the same we have in l10n-en_GB.js) with the JS selector that will retrieve the user-friendly, verbose description of a rule. These selectors are applied to rules.json calling eval() (yes, eval()!). These user-friendly descriptions are the ones you see in the old pubrules checker.
  • And this new verbose explanation about a rule is shown in Specberus' UI now.
  • Added 3 new tests to make sure that all values in l10n-selectors.js resolve correctly to strings in rules.json. This is to make the whole thing a bit more robust — writing selectors like this, and passing them along to eval() would be error-prone otherwise.

Work that is pending, and improvements:

  • @deniak's program to generate rules.json ideally should produce this version (a large object, instead of a large array).
  • Continue writing selectors for all rules (there are only 6 now).
  • At some point, translate messages to other languages? (this isn't even discussed, and has no priority whatsoever at the moment).
  • Switching between different languages (if ever we support several) should be done on the UI, by the user (not hard-coding the locale in validator.js).
  • Get rid of eval(); do it better.
  • Right now, l10n-selectors.js assumes that the profile is WD. When we start supporting other types of docs in Echidna, we'll have to refactor this, so that the profile is a parameter.
  • Come up with a better way to maintain rules, selectors and messages!

@tripu
Copy link
Member Author

tripu commented Mar 10, 2015

@deniak, assigning to you because at least you saw the beginning of this work weeks ago. I know I haven't done a good job at explaining the changes, so please ask me about this. And if you can think of simpler, safer ways to do this, let me know too!

@astorije
Copy link
Member

@tripu, that's a lot of commits up there. Did you mess something up on your local copy? You probably don't want to have all of them merged and squash some of them before merging.

@deniak
Copy link
Member

deniak commented Mar 12, 2015

+1 to squash the commits

@deniak deniak force-pushed the tripu/architectural-changes branch 2 times, most recently from 5e2a2f8 to 53bc74c Compare March 12, 2015 04:34
astorije and others added 2 commits March 12, 2015 12:43
Set locale in "lib/validator.js".

Added Denis' JSON dump of all rules.

Conflicts:
	lib/l10n-en_GB.js
	lib/l10n-es_ES.js
	lib/l10n.js
	lib/rules.json
	lib/validator.js
@deniak deniak force-pushed the tripu/architectural-changes branch from 53bc74c to e6d47f4 Compare March 12, 2015 04:43
@astorije
Copy link
Member

Hum, the fact that an old commit of mine (aka already in master) appears in this PR is very likely (in my understanding) to ask to rewrite the history in master :-/

@tripu tripu force-pushed the tripu/architectural-changes branch from 9672c17 to 21113b3 Compare March 13, 2015 02:11
@tripu tripu merged commit 7bbc679 into master Mar 13, 2015
@tripu tripu deleted the tripu/architectural-changes branch March 13, 2015 02:42
@astorije
Copy link
Member

I'm a bit unclear: does this PR add any feature? Spanish support, even if poor, seems one. If so, maybe this should have triggered version 1.1.0 instead of 1.0.2 but I agree it's borderline here.

That said... urgh to eval() indeed! I didn't think this PR would make it to master until it's being removed :-(

@tripu
Copy link
Member Author

tripu commented Mar 13, 2015

@astorije:

Having these two messages in Spanish serves mainly as a test to prove that the new i18n feature works. i18n is a byproduct of the refactoring I've done here. Actually translating the tool is not something I intend to do any time soon…

The actual feature is that we can start showing detailed, formatted descriptions for rules (exactly the same texts you see in the old pubrules checker) by simply filling in all the selectors in lib/l10n-selectors.js. Because most of those selectors still need to be written, I wouldn't consider this even a “minor” change yet — once they're all there, we can increase the minor, sure.

Finally, those selectors are the reason I'm using eval(). Sure, it's bad practice. I'd like to find a more elegant way to maintain rules and descriptions, and match ones against the others. That being said, eval() here is being used in a very specific context, and I certainly can live with it.

tripu pushed a commit that referenced this pull request May 19, 2015
deniak added a commit that referenced this pull request May 20, 2015
Fix copy removed accidentally around PR #160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants