Closed Bug 801665 Opened 8 years ago Closed 8 years ago

Provide error recovery for parser

Categories

(L20n :: JS Library, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file, 1 obsolete file)

If L20n parser encounters a ParserError it should be able to recover and start again from the next closest entity.
Assignee: nobody → gandalf
Blocks: 802846
Attached patch parser recovery patch (obsolete) — Splinter Review
patch v1
Attachment #675058 - Flags: review?(stas)
Comment on attachment 675058 [details] [diff] [review]
parser recovery patch

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

r=me with comments (see below).

::: lib/l20n.js
@@ +7,4 @@
>  
>    if (typeof exports !== 'undefined') {
>      L20n = exports;
> +    L20n.EventEmitter = require('./eemitter.js');

Can you call it 'events.js'?

@@ +199,5 @@
> +        parser.addEventListener('error', function(e) {
> +          debug('Parser error in ' + id);
> +          debug('Error msg is ' + e.msg);
> +          debug('Error around "' + e.context + '"');
> +        });

We should bubble an Exception up to the ctx here, I think.

if (env.DEBUG) {
  exception = new Exception(L20n.PARSE_ERROR, 'Non-fatal parser error in ' + id, e);
  _fire(fallbacks, [exception, true]);
}

The second argument, 'true', is responsible for calling the fallbacks in a no-op way.  

Can you check if this works as intended?  If it does, r=me.

@@ +208,3 @@
>          _fire(fallbacks, [exception]);
>          return false;
>        }

What happens if none of the entities defined in a resource is parsed correctly?  Do we just show debug output, or should we consider the resource as corrupt?  Sounds like bug 801663?

::: lib/parser.js
@@ +1,5 @@
>  (function() {
>    'use strict';
>  
> +  function Parser(L20n) {
> +    var emitter = new L20n.EventEmitter();

To avoid passing L20n to the Parser constructor, I think you could do the 'typeof exports' check before the Parser function definition and 'require' EventEmitter if needed, or check for 'this.L20n'.

It probably doesn't matter much, because bug 805163 will solve this anyways.

::: tests/lib/context.js
@@ +374,5 @@
>    });
>  
> +  describe('Broken lol', function(){
> +    before(function() {
> +      server.scenario = 'broken_lol';

Please remember to commit this LOL to lol-fixtures and update the submodule.
Attachment #675058 - Flags: review?(stas) → review+
Priority: -- → P2
Target Milestone: --- → 1.0
No longer blocks: 802846
Depends on: 801663
Duplicate of this bug: 801668
Blocks: 802850
Comment on attachment 675058 [details] [diff] [review]
parser recovery patch

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

Actually, I'm going to r- this because of the following line I just spotted.

::: lib/l20n.js
@@ +195,4 @@
>  
>      function parse(data) {
>        try {
> +        var parser = new L20n.Parser(L20n);

This doesn't look right.  We should create one parser per context.  In which case I'm not sure if we should have a single cache for downloaded files shared by all contexts.
Attachment #675058 - Flags: review+ → review-
We should keep single cache for downloaded files but cache only if the LOL parses without errors.
Status: NEW → ASSIGNED
Or, cache the raw text, but not the AST, on error?
Attached patch patch v2Splinter Review
updated patch:
 - comments addressed
 - optional parameter to define reaction to error
Attachment #675058 - Attachment is obsolete: true
Attachment #678955 - Flags: review?(stas)
Comment on attachment 678955 [details] [diff] [review]
patch v2

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

Sweet, r=me.  Let's file follow-up bugs about some of the issues we talked about today and that I mentioned below.

::: lib/l20n.js
@@ +194,4 @@
>      }
>  
>      function parse(data) {
> +      isDownloading = false;

This means that each resource file will be re-downloaded every time get() is called.  I understand why this is so, but can you please file a follow-up bug to make sure we fix how we call `parse` as a callback to `get` and `download`?

@@ +198,1 @@
>        try {

Can you move everything but the parse() call out of the try clause?

@@ +208,5 @@
> +            exception = new Exception(L20n.PARSE_ERROR, 'Non-fatal parser error in ' + id, e);
> +            _fire(fallbacks, [exception, true]);
> +          }
> +          hasErrors = true;
> +          // we should fire a fallback here to emit error in context?

Is this a question, or a comment? :)

@@ +222,4 @@
>          return elem.type == 'ImportStatement';
>        });
> +      if (!hasErrors) {
> +        ast = rast;

This suggests to me that hasError should be defined in the scope of the whole resource and be used in get() together with the check for `ast`.  No need to `rast` then.

Something like this maybe?

 this.get = function r_get(callback, fallback, async) {
      if (ast && !hasErrors) {
        callback(ast, imports);
      } else if (isCorrupted) {
        fallback(exception);
      } else {

@@ +596,4 @@
>        if (self.settings.locales && self.settings.locales.length > 1) {
>          subctx.settings.locales = self.settings.locales.slice(1);
>        } else {
> +        // this should throw "entity not available"

Mark it with XXX or TODO and file a bug please?
Attachment #678955 - Flags: review?(stas) → review+
Filed bugs, updated comments and pushed! Thanks :)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.