search

openaustralia / jacaranda

a watchful tree and slack messenger to keep you informed of the use of your civic tech projects, like PlanningAlerts and Right To Know
https://morph.io/openaustralia/jacaranda
0 stars 1 forks source link

Add Right To Know stats, and put runners in one place #8

Closed auxesis closed 7 years ago

auxesis commented 7 years ago

Looks like this:

image

This effectively deprecates the need for openaustralia/jacaranda-righttoknow.

Also:

The intention is to make Jacaranda easier to understand and contribute to.

equivalentideas commented 7 years ago

@auxesis for me this is clearer 👍

There's some other feedback on smaller stuff on the other PR that would be good to bring over, and does the readme have to update for this version?

I'm also interested in why we're adding the command line options stuff to this, rather than have it controlled through environment variables for morph.

I know you're a command line wizard ;D but the need isn't apparent to me. Have you got a specific use case in mind?

If you're happy with this and have looked at the feedback on the other PR I'm happy for you to merge this 👍 don't wanna hold it up.

auxesis commented 7 years ago

Feedback from #5 that needs to be checked and incorporated:

auxesis commented 7 years ago

@equivalentideas

Have added a line to the README explaining why the --runners option was added.

The only reason it's there is to aid debugging when contributors are adding new runners. I definitely wouldn't expect it to be used this way when it's run on Morph. You're right though – we could change this to be an environment variable, given we're already gone all in on dotenv.

I'll update the rest of the README to match the new way of adding runners.

equivalentideas commented 7 years ago

The only reason it's there is to aid debugging when contributors are adding new runners. I definitely wouldn't expect it to be used this way when it's run on Morph. You're right though – we could change this to be an environment variable, given we're already gone all in on dotenv.

That sounds reasonable to me @auxesis 👍 thanks as always 💓

auxesis commented 7 years ago

@equivalentideas this is all fixed up now.

Let me know if you're good with the last few documentation changes, and I'll merge.

auxesis commented 7 years ago

I've had to do some fairly significant refactoring of the tests, and things are broken again.

Will come back to this when I have time.

auxesis commented 7 years ago

@equivalentideas have refactored the tests per your feedback. Ready for another review.

equivalentideas commented 7 years ago

I'm merged this @auxesis thanks so much for this huge contribution. I'll disable the Right To Know jacaranda on morph.io now and just let this one run.

auxesis commented 7 years ago

Argh. After all this work, I just realised that there's caveats listed in #5 that I haven't addressed in this PR:

Caveats:

This PR changes the primary key from date_posted to a composite primary key of date_posted and runner. I need to do further testing to see how the scraper handles this when run on Morph (the testing behaviour locally was not great).

I'll do a quick test this evening and double check what happens.

If I discover problems, I'll submit another PR.

equivalentideas commented 7 years ago

Argh. After all this work, I just realised that there's caveats listed in #5 that I haven't addressed in this PR

All good @auxesis with such a big PR changing so much it's so easy to miss stuff like that.

auxesis commented 7 years ago

Forgot to update last night, but there are problems: #15.