Tar widget with hints

Tar widget with hints

[s.z.w.lip, 2013-09-26 07:48:30]

Hi Sunu,

I think there were a bunch of comments I made in the previous review that
weren't addressed at all; could you please look through that as well and make
sure everything is taken care of?

Thanks,
Sean

https://codereview.appspot.com/13884044/diff/1/extensions/rules/tar_file_stri...
File extensions/rules/tar_file_string.py (right):

https://codereview.appspot.com/13884044/diff/1/extensions/rules/tar_file_stri...
extensions/rules/tar_file_string.py:96: != utils.get_file_contents(
I thought we were going to try to pull some of this out and make it more
readable?

https://codereview.appspot.com/13884044/diff/1/extensions/rules/tar_file_stri...
File extensions/rules/tar_file_string_test.py (right):

https://codereview.appspot.com/13884044/diff/1/extensions/rules/tar_file_stri...
extensions/rules/tar_file_string_test.py:30: """Tests for rules operating on
UnicodeString objects."""
Need more tests for the additional rules.

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
File
extensions/widgets/interactive/FileReadInputWithHints/static/js/FileReadInputWithHints.js
(right):

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
extensions/widgets/interactive/FileReadInputWithHints/static/js/FileReadInputWithHints.js:31:
$('#processing-modal > .modal-body > p').html("Processing uploaded file: " +
theFile.name);
Can we move this to the html?

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
File
extensions/widgets/interactive/TarFileReadInputWithHints/TarFileReadInputWithHints.py
(right):

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
extensions/widgets/interactive/TarFileReadInputWithHints/TarFileReadInputWithHints.py:6:
class TarFileReadInputWithHints(widget_domain.BaseWidget):
Argh, so much duplication. But I see why you're doing this; it's because we
can't parameterize the output type.

Fair warning: this will change in the future and I will try to merge this with
the other FileReadInput widget (as well as break the hints out into something
separate). But I think we can accept this for now.

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
extensions/widgets/interactive/TarFileReadInputWithHints/TarFileReadInputWithHints.py:66:
]
Please move ] to previous line (I think that is more readable).

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
File
extensions/widgets/interactive/TarFileReadInputWithHints/static/js/TarFileReadInputWithHints.js
(right):

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
extensions/widgets/interactive/TarFileReadInputWithHints/static/js/TarFileReadInputWithHints.js:24:
alert("File too large. Please choose a file less than 200 kilobyte in size.")
This is way too big. Can we have something smaller?

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
extensions/widgets/interactive/TarFileReadInputWithHints/static/js/TarFileReadInputWithHints.js:31:
$('#processing-modal > .modal-body > p').html("Processing uploaded file: " +
theFile.name);
As before, can we do this in the html instead?

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
extensions/widgets/interactive/TarFileReadInputWithHints/static/js/TarFileReadInputWithHints.js:42:
console.log(data)
Why did you remove the semicolon?

https://codereview.appspot.com/13884044/diff/1/extensions/widgets/interactive...
extensions/widgets/interactive/TarFileReadInputWithHints/static/js/TarFileReadInputWithHints.js:45:
alert("An error occurred while processing your input.")
Ditto.

[sunu0000, 2013-09-26 16:39:35]

Hi Sean!

Updated the review. Fixed things I had missed before(sorry!).
See if I missed anything.

Thanks.

[s.z.w.lip, 2013-09-27 05:38:16]

Hi Sunu!

Thanks, I think this looks good. After you address all the comments, please make
sure that all tests pass, and also do some manual clicking to make sure
everything works, then feel free to submit.

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
File extensions/rules/tar_file_string.py (right):

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
extensions/rules/tar_file_string.py:51: description = 'Contains a Apple Double
file.'
s/a/an

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
extensions/rules/tar_file_string.py:82:
set(self.expected_files)-set(member_list)
Please add a single space around the '-' operator.

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
extensions/rules/tar_file_string.py:99: if (subj_contents != expected_contents):
Suggest removing the brackets here.

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
File extensions/rules/tar_file_string_test.py (right):

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
extensions/rules/tar_file_string_test.py:29: class
TarFileStringRuleUnitTests(unittest.TestCase):
Did you change the number of tests in the test suite?

(Please make sure that every test passes before submitting your code.)

https://codereview.appspot.com/13884044/diff/9001/extensions/widgets/interact...
File
extensions/widgets/interactive/FileReadInputWithHints/static/js/FileReadInputWithHints.js
(right):

https://codereview.appspot.com/13884044/diff/9001/extensions/widgets/interact...
extensions/widgets/interactive/FileReadInputWithHints/static/js/FileReadInputWithHints.js:33:
$scope.$apply();
Just checking: why do you need this? (What does not work?)

Please add a comment to the file explaining this -- we are trying to remove
$scope.$apply()'s in various places so it would help to have the context.

[sunu0000, 2013-09-28 06:13:22]

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
File extensions/rules/tar_file_string_test.py (right):

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
extensions/rules/tar_file_string_test.py:29: class
TarFileStringRuleUnitTests(unittest.TestCase):
On 2013/09/27 05:38:17, s.z.w.lip wrote:
> Did you change the number of tests in the test suite?
> 
> (Please make sure that every test passes before submitting your code.)

Err... one test is failing. The name of the parameter in widget doesn't allow
'_'. Should I change the name or should we include '_' as well?

https://codereview.appspot.com/13884044/diff/9001/extensions/widgets/interact...
File
extensions/widgets/interactive/FileReadInputWithHints/static/js/FileReadInputWithHints.js
(right):

https://codereview.appspot.com/13884044/diff/9001/extensions/widgets/interact...
extensions/widgets/interactive/FileReadInputWithHints/static/js/FileReadInputWithHints.js:33:
$scope.$apply();
On 2013/09/27 05:38:17, s.z.w.lip wrote:
> Just checking: why do you need this? (What does not work?)
> 
> Please add a comment to the file explaining this -- we are trying to remove
> $scope.$apply()'s in various places so it would help to have the context.he 

The filename originally is an empty string. When the filename gets updated, the
updated value doesn't show up in the browser. So I'm doing $scope.$apply()

[s.z.w.lip, 2013-09-28 06:53:39]

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
File extensions/rules/tar_file_string_test.py (right):

https://codereview.appspot.com/13884044/diff/9001/extensions/rules/tar_file_s...
extensions/rules/tar_file_string_test.py:29: class
TarFileStringRuleUnitTests(unittest.TestCase):
Ah. Yes, I think we can allow '_'.

On 2013/09/28 06:13:22, sunu0000 wrote:
> On 2013/09/27 05:38:17, s.z.w.lip wrote:
> > Did you change the number of tests in the test suite?
> > 
> > (Please make sure that every test passes before submitting your code.)
> 
> Err... one test is failing. The name of the parameter in widget doesn't allow
> '_'. Should I change the name or should we include '_' as well?

https://codereview.appspot.com/13884044/diff/9001/extensions/widgets/interact...
File
extensions/widgets/interactive/FileReadInputWithHints/static/js/FileReadInputWithHints.js
(right):

https://codereview.appspot.com/13884044/diff/9001/extensions/widgets/interact...
extensions/widgets/interactive/FileReadInputWithHints/static/js/FileReadInputWithHints.js:33:
$scope.$apply();
Thanks for explaining! Hmm, that is actually unexpected. Can you put a comment
in this and the other JS file explaining this:

   // The call to $scope.$apply() is needed because $scope.filename does not
   // update automatically in the HTML template.

Then, one day in the future, I'll look into it :-)

Thanks!

On 2013/09/28 06:13:22, sunu0000 wrote:
> On 2013/09/27 05:38:17, s.z.w.lip wrote:
> > Just checking: why do you need this? (What does not work?)
> > 
> > Please add a comment to the file explaining this -- we are trying to remove
> > $scope.$apply()'s in various places so it would help to have the context.he 
> 
> The filename originally is an empty string. When the filename gets updated,
the
> updated value doesn't show up in the browser. So I'm doing $scope.$apply()  
>