AutoReviewComment script devel

10:50 AM

I have a (very) basic start to the #6 issue going in in a feature branch that has release stuff in it too, so it is install-able as a userscript. Right now all it does is default the remote field to a per-site url and has default sets for a couple of sites.

Caleb

11:01 AM

@Benjol My vision is something like:

* Notice first time users per-site; instead of loading default comments, check to see if a per-site json URL exists
** if 404, load the default set, one time prompt the user that a per-site set was not found, and link them to a wiki page for how to contribute per site sets
** If 200, etc download the json data, merge it with the default set json, and prompt the user that a per-site comment set has been found and loaded, one time prompt the user about how to contribute changes back
* on subsequent visits

Izzy

11:17 AM

@Caleb The last item might prove a little difficult, as it might produce duplicates. Except, of course, you already have considered and handled this somehow (I'm currently not able to follow the whole line, as I'm quite busy on other tasks, sorry).

Caleb

@Izzy Why would it produce duplicates? The per site data refreshed from the remote URL plus the in-program default set should never produce a duplicate, and the "auto-update" would nuke any customizations made locally (that could be a warning given when enabling the option).

Also @Izzy You are active on the Android site right? Is there a better comment set for that site to seed the user contrib folder with than the one I used?

Izzy

11:47 AM

@Caleb always merge the script defaults into the end of whatever set is remote You speak of merge, not replace. How will you match existing comments? What about those the user added himself manually, or modified?

@Caleb Yes, I am. And yes, I've got some site-specific stuff for there, too. I even posted it somewhere (for sure in the edit history of that SR meta post).

Checking your link...

@Caleb Yepp, there are some comments missing. I could quickly pastebin some of mine if you like.

Caleb

@Izzy By merge I mean <join>. Since both sets will be it our repo we can know that the contrib folder won't have anything duplicated with the stock. If they load a custom URL we can skip that join. There is no such thing as "existing comments" because the load from remote will nuke all current customized ones.

Izzy

@Caleb OK, so you can only (auto-)update from that remote url XOR make your own modifications?

Caleb

@Izzy Sure. Or branch that feature branch and commit them directly.

@Izzy Yes, those would be mutually exclusive usage patterns.

Hence the "prompting the user for how to contribute" if they make local modifications so eventually they could go back to using the default remote pull.

(Always assuming their comments don't suck, it which case they won't be accepted by whoever is coordinating that site's default comment set and they are on their own for updating their sucky comments.)

Izzy

@Caleb Can't. I'm not home, so no access to git tools. You want only the site specific ones, correct?

Caleb

@Izzy That's fine. And yes, only site specific ones.

Izzy

12:01 PM

Because that diff you linked also contains generic ones. Will that be separate sets, or should I include diffs here as well?

Izzy

12:18 PM

@Caleb Here's my update: pastebin.mozilla.org/4539182

I've copied all comments from your diff, compared them with mine, and updated them (all). Some more added as well.

Benjol

12:42 PM

@Caleb If I understand correctly, the default comments would be embedded in the script?

That way 404 wouldn't kill them.

But do you really need default comments if you've got the custom ones?

Caleb

@Benjol Yes. Saves a network call and possible completely empty comments scenario if 404.

Benjol

Do you see all these custom scripts living in the same place?

(on github)?

Caleb

@Benjol Probably not, but I noticed a number of sites included the stock ones along with their shared ones.

Benjol

or each has their own repo?

I'm thinking that if you want to avoid duplicates, they might want to pull the default comments in from 'our' repo to theirs?

I guess I only have two (minor) concerns: a) that own modifications could be easily overridden (but I'm prepared to live with that compared to the 'merging' alternative). And b) that we manage to make it not-too-confusing for the common mortal.

Caleb

@Benjol Probably one repo with either pulls from forks that are the managed by whatever site rep takes initiative or by adding contributors to a repo so somebody from each site can handle contributions. I like the tree layout with two levels of forks and a little bit of vetting for what ends up in the app defaults.

Benjol

12:46 PM

@Caleb, definitely agree with vetting

'Scuse me while I pull your branch and have a poke around

@Caleb, ok, looks quite good. I like that your minimal code change already gets us quite a lot of the way there :)

Do you think it should be first time users of the script, or first time users of this version of the script? I suspect there are quite a lot of long-time users who've never noticed that remote exists

Caleb

@Benjol Not much functional change is needed, the biggest thing is changing the default flow and prompts for getting folks on board.

@Benjol The latter actually, but with a special notice if they have any changes in localstorage that they have made would be overwritten if they choose to pull from a remote.

Benjol

@Caleb, ok.

Izzy

1:02 PM

@Caleb did you pick my pastebin, or shoud I reserve it?

Caleb

1:14 PM

@Izzy Got it.

Izzy

@Caleb OK, thanks! Then I can close that tab without fear of losing something :) Hope you found it useful :)

Benjol

1:29 PM

@Caleb, nice catch with the location.protocol too

Caleb

@Izzy github.com/alerque/SE-AutoReviewComments/commit/…

@Benjol That was Derek's catch, but my implementation since he hacked it before posting the Chrome extension (see #9 discussion).

Izzy

@Caleb C00l!

Btw: I've meanwhile got that many comments in my setup, I need to scroll. Brainstorming: some feature to collapse, but auto-expand comments on mouse-over might come in handy...

Benjol

@Caleb, you could have added yourself to the credits!

Caleb

@Benjol I did.

Benjol

1:44 PM

oh

Caleb

Order of appearance, so I'm near the bottom.

Benjol

I missed it, let me look again :)

doh

@Caleb, are we ok for merging #29?

Caleb

@Benjol #29 is already merged, #30 (same thing but to master) needs to wait until we have confirmed that Derek has fixed his mis-step posting a hacked script with the full one to the Chrome Store. That has to happen first or it the auto-update code (which shouldn't have been in there) will prompt folks for a broken update system.

One the store is up to 1.3.3.1 we should probably wait a day to merge #30 and reflect what the latest release actually is.

Benjol

@Caleb, ok, yeah misread my issue number

I'll hold off on 30 then (just merged #32)

Benjol

2:14 PM

@Caleb github.com/Benjol/SE-AutoReviewComments/issues/…

Transcript for

AutoReviewComment script devel