ABP/Android subscription list

ABP/Android subscription list

[Felix Dahlke, 2012-11-13 09:58:12]

Just one small issue here.

http://codereview.adblockplus.org/8491080/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/SubscriptionParser.java (right):

http://codereview.adblockplus.org/8491080/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/SubscriptionParser.java:19: private
List<Subscription> subscriptions;
How about naming this "currentSubscription" to highlight that it's the one being
currently parsed?

[Felix Dahlke, 2012-11-13 09:58:59]

Make that two :)

http://codereview.adblockplus.org/8491080/diff/1/assets/subscriptions.xml
File assets/subscriptions.xml (right):

http://codereview.adblockplus.org/8491080/diff/1/assets/subscriptions.xml#new...
assets/subscriptions.xml:70: </subscriptions>
This shouldn't be indented.

[Andrey Novikov, 2012-11-14 07:13:59]

http://codereview.adblockplus.org/8491080/diff/1/assets/subscriptions.xml
File assets/subscriptions.xml (right):

http://codereview.adblockplus.org/8491080/diff/1/assets/subscriptions.xml#new...
assets/subscriptions.xml:70: </subscriptions>
On 2012/11/13 09:58:59, Felix H. Dahlke wrote:
> This shouldn't be indented.

It's a copy of
https://hg.adblockplus.org/adblockpluschrome/file/a471d73eb8a2/subscriptions.xml
I think Wladimir generates it somehow?

http://codereview.adblockplus.org/8491080/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/SubscriptionParser.java (right):

http://codereview.adblockplus.org/8491080/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/SubscriptionParser.java:19: private
List<Subscription> subscriptions;
On 2012/11/13 09:58:12, Felix H. Dahlke wrote:
> How about naming this "currentSubscription" to highlight that it's the one
being
> currently parsed?

Do not see any reason but done.

[Felix Dahlke, 2012-11-14 09:49:05]

On 2012/11/14 07:13:59, Andrey Novikov wrote:
> http://codereview.adblockplus.org/8491080/diff/1/assets/subscriptions.xml
> File assets/subscriptions.xml (right):
> 
>
http://codereview.adblockplus.org/8491080/diff/1/assets/subscriptions.xml#new...
> assets/subscriptions.xml:70: </subscriptions>
> On 2012/11/13 09:58:59, Felix H. Dahlke wrote:
> > This shouldn't be indented.
> 
> It's a copy of
>
https://hg.adblockplus.org/adblockpluschrome/file/a471d73eb8a2/subscriptions.xml
> I think Wladimir generates it somehow?

OK, I see.

>
http://codereview.adblockplus.org/8491080/diff/1/src/org/adblockplus/android/...
> File src/org/adblockplus/android/SubscriptionParser.java (right):
> 
>
http://codereview.adblockplus.org/8491080/diff/1/src/org/adblockplus/android/...
> src/org/adblockplus/android/SubscriptionParser.java:19: private
> List<Subscription> subscriptions;
> On 2012/11/13 09:58:12, Felix H. Dahlke wrote:
> > How about naming this "currentSubscription" to highlight that it's the one
> being
> > currently parsed?
> 
> Do not see any reason but done.

The two variables are very similar in name, I was staring at them for about a
minute, trying to figure it out. Only then did I read the rest of the code and
noticed that subscription was the currently processed subscription, while
subscriptions where all of them.

Anyway, LGTM.

[Wladimir Palant, 2012-11-14 09:55:19]

http://codereview.adblockplus.org/8491080/diff/1/assets/subscriptions.xml
File assets/subscriptions.xml (right):

http://codereview.adblockplus.org/8491080/diff/1/assets/subscriptions.xml#new...
assets/subscriptions.xml:70: </subscriptions>
Yes, it's autogenerated and the indentation is an artifact of the Jinja2
template.