667900 - [shipping] make diff view show parsed entities for added and removed files, too

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Axel Hecht [:Pike]]

The current diff view (in shipping, still) only shows links to the plain files for added and removed files.

This grew to be a PITA with the rapid release process, as we're doing incremental reviews with added and removed files now.

Should be simple to fix, I'll take a stab at this.

Making this a P2, as it's dogfood.

Comment 1

[Axel Hecht [:Pike]]

Attachment: do rich diff for new and removed files, too

Parse also removed and added files.

This patch doesn't come with tests, there are no existing tests, and adding tests would conflict with the externalize media patch, as the diff tests are different enough to go into their own file in (then) tests/diff.py.

We should do real tests for this code in a follow up.

Comment 2

[Peter Bengtsson [:peterbe]]

Comment on attachment 543674 [details] [diff] [review]
do rich diff for new and removed files, too

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

I love the final result of this patch! Not that I've actually tested it but it's going to make the diff view much better for users. Great work!

Yes, we need tests for this but you're right, things will change quite a bit with the scaffolding I put in place on the externalize-all-media branch. Do you want me to create a new bug for writing these tests and assign it to you? It doesn't need a uber-detailed test. Just a little integration test so that if we accidentally upgrade to a broken parser it'd be caught in the test runner instead of having to see actual breakage in the browser. 

Giving it an r- because of the empty exceptions which I'm aware were there before too but this should be a chance to correct that.

::: apps/shipping/views/__init__.py
@@ +139,5 @@
> +            a_entities = []
> +            a_map = {}
> +        else:
> +            data = ctx1.filectx(path).data()
> +            data = __universal_le(data)

I know this was already in there before this patch but...

Why the cryptic name and the dunder prefix? Single dunder prefix should only be used for classes where you don't want private methods to be inherited. 

Also, the name isn't reader-friendly. How about "_fix_line_endings()"? Or "_to_unix_line_endings()"

@@ +143,5 @@
> +            data = __universal_le(data)
> +            p.readContents(data)
> +            try:
> +                a_entities, a_map = p.parse()
> +            except:

Empty exception :(

We should avoid this at all cost. The *only* time it's acceptable is if you're relying on a third party library that is behaving weirdly and if this is the case, there should be a comment saying something like "Due to the shittiness of this bizarre library we have to catch all exceptions. sorry :)"

I think it should be something like this:

  try:
      a_entities, a_map = p.parse()
  except Mozilla.SomeSpecificParseError:
      # happens if the file has been removed and it's normal

@@ +148,5 @@
> +                diffs[path].update({'path': path,
> +                                    'isFile': True,
> +                                    'rev': ((action == 'removed') and request.GET['from']
> +                                            or request.GET['to']),
> +                                    'desc': ' (cannot parse file)',

Should we perhaps push this into the view template instead. If the desc is None we can write something in English about it in the template. No biggie for me.

@@ +161,5 @@
> +            data = __universal_le(data)
> +            p.readContents(data)
> +            try:
> +                c_entities, c_map = p.parse()
> +            except:

Again, empty exception.

@@ +169,5 @@
> +                                            or request.GET['to']),
> +                                    'desc': ' (cannot parse file)',
> +                                    'class': action})
> +                continue
> +        del p

Why this? Is the parser leaking memory? It's only going to live until the end of the rendering of the template and then be cleaned up anyway. 
If there is a leakage problem perhaps a little comment like...

 # Important! The parser is leaking and must be collected before rendering
 # because of a nasty old bug.
 del p

Comment 3

[Axel Hecht [:Pike]]

Re the empty exceptions:

There is no expected exception, but this is code that's fuzz tested against l10n files. There's a good fallback option for anything going wrong during the parsing step, so we're using that fallback step in case.

Also, the library is my compare-locales, and that doesn't make any effort to only throw particular exceptions, and won't for a while. That's probably OK because errors in user input should result in an error token returned as part of a successful parse, so we're really just dealing with the spanish inquisition here.

Comment 4

[Peter Bengtsson [:peterbe]]

(In reply to comment #3)
> Re the empty exceptions:
> 
> There is no expected exception, but this is code that's fuzz tested against
> l10n files. There's a good fallback option for anything going wrong during
> the parsing step, so we're using that fallback step in case.
> 
> Also, the library is my compare-locales, and that doesn't make any effort to
> only throw particular exceptions, and won't for a while. That's probably OK
> because errors in user input should result in an error token returned as
> part of a successful parse, so we're really just dealing with the spanish
> inquisition here.

I guess it's ok. It's only one line after all. I've used blank excepts some times when calling on a compiled binary where you can't easily get the exception classes out. 

My main concern with blank excepts is that they can potentially hide "built in exceptions" such as TypeError, IndexError or NameError if the API changes but you forget change the code that uses it. For example, if 'parse()' method one day returns a tuple of 3 items. In this case, very unlikely to happen.

Comment 5

[Axel Hecht [:Pike]]

Attachment: address review comments

As per previous discussion, I'm leaving the unconstrained exception for parsing in. I've also put in the readContents() calls in there, as they're doing the decoding of utf-8, and thus can bail. This is also what the code used to do.

I've renamed __universal_le to _universal_newlines(), as that's what the 'U' python mode is called.

The del is gone, that's not good for anything as I'm keeping the parser instance cached anyway.

A follow-up bug for tests would be good, I've got a good idea on what to do there. We may want to fix bug 562204 first, though, as we'd have less code to move then.

Comment 6

[Peter Bengtsson [:peterbe]]

Comment on attachment 547361 [details] [diff] [review]
address review comments

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

Looks much better

::: apps/shipping/views/__init__.py
@@ -159,0 +132,28 @@
> > +        if action == 'added':
> > +            a_entities = []
> > +            a_map = {}
> > +        else:
NaN more ...

An idea, whenever you have a swallowed exception is to log it. E.g.:

  except:
      import logging # unless imported already
      logging.error("Failed to do <foo>", exc_info=True)
      ...

Comment 7

[Peter Bengtsson [:peterbe]]

(In reply to comment #5)
> 
> A follow-up bug for tests would be good, I've got a good idea on what to do
> there. We may want to fix bug 562204 first, though, as we'd have less code
> to move then.

Happy to help there but because the code is quite complex in terms of business logic I'd rather you take the lead on the unit testing.

Comment 8

[Axel Hecht [:Pike]]

Filed bug 673935 on the tests, marking FIXED as this landed on develop, https://github.com/mozilla/elmo/commit/43adb392daf0d4e8bac00f18f2e01ece455b966e