RequestPolicyContinued / subscriptions

subscriptions for RequestPolicyContinued
http://requestpolicycontinued.github.io/
Other
19 stars 8 forks source link

Fixed indentation #6

Closed reqsub closed 9 years ago

nodiscc commented 9 years ago

@reqsub thanks for this. It's a large change so it will take some time for to review all of it. Unless there is a way to automate tests (I can't think of one). By the way how did you do it? Is it only manual site visits/changes with the RequestPolicy menu? I could add the method to the README to to help other contributors.

If you want to add more changes, please create another branch, and do it there (so it will not add the changes to this pull request)

If you are not familiar with git branches just let me know. There's a good starting tutorial at https://try.github.io/

myrdd commented 9 years ago

Hi @reqsub, thanks for participating! :) Please be patient with the review process, because we're just at the beginning. Any ideas on how to organize subscriptions or related to the project in general are very welcome!

@nodiscc please note that when updating a subscription file you also need to update the subscription's "serial" in the "subscription list file" (official.json). For details, see this code comment. I suggest to change the serial to something that at least contains the date (e.g. 2014111101 for a change done on 2014-11-11. If you make another change on the same day, increment by one.). Note also that this should be done by the person who merges the pull request, not by the person that makes the pull request, as the date might change due to delayed review.

Another note @nodiscc (sorry for the long text), IMHO we should define very clearly what to put into a subscription and what not to include before we do big changes. See also @drzraf's comment: https://github.com/RequestPolicyContinued/requestpolicy/issues/339#issuecomment-62312286. I'll write about my thoughts some other time.

nodiscc commented 9 years ago

@myrdd ok, so the serial must be bumped to trigger updates, noted.

we should define very clearly what to put into a subscription and what not to include before we do big changes

https://github.com/RequestPolicyContinued/requestpolicy/issues/339#issuecomment-62312286 not everyone accept the same restriction from policy, for example, an external script may trigger a slideshow of a news website what is acceptable to be blocked for one, but not for another, I think there is place for distinct "policies" here.

@myrdd @reqsub I think changes should be kept to a minimum to just enable the pages to render properly. For example I am ok to add {"o":{"h":"*.ebay.de"},"d":{"h":"*.ebaystatic.com"}} as it affects the page rendering, but not {"o":{"h":"*.chromium.org"},"d":{"h":"*.google.com"}} as chromium.org works fine without requests to google.com.

Does it make sense?

nodiscc commented 9 years ago

@reqsub I will review your changes and hand pick what's necessary when https://github.com/RequestPolicyContinued/subscriptions/pull/11 is merged.

nodiscc commented 9 years ago

@reqsub as you can see your changes were very hard to review because you didn't preserve the original file ordering. There were also some rules that do not fit _allowsameorg (not the same organization), or rules that are not necessary to make the sites work/display properly.

Anyway, I added some of your changes to my pull request in https://github.com/RequestPolicyContinued/subscriptions/pull/11, thanks for your work.

I'll close this PR, feel free to open another one if you want to add more changes - I suggest waiting for https://github.com/RequestPolicyContinued/subscriptions/pull/11 to be merged, and then basing your future changes on this file, but do not change the order of rules or it will be very hard to review (as you can see in https://github.com/RequestPolicyContinued/subscriptions/pull/6/files the diff is a mess).