Skip to content

Commit

Permalink
Explain why logic is in SQL and ask for help
Browse files Browse the repository at this point in the history 
Now we have external contributors (hi @chadmiller!), it's time to
start thinking more about comprehensibility
  • Loading branch information
@sebbacon
sebbacon committed Mar 12, 2018
1 parent 47409c2 commit ce0c22d
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions README.md
View file
Expand Up @@ -42,6 +42,13 @@ loaded into a staging database / website.
A separate command copies new data from staging to production
(following moderation).

Much complex logic has been expressed in SQL, which makes it hard to read
and test. This is a legacy of splitting the development between
academics with the domain expertise (and who could use SQL to
prototype) and software engineers. Now the project has been running
for a while and new development interations are less frequent, a useful
project would be as much of this logic to Python.

Static Pages
============

Expand Down

6 comments on commit ce0c22d

@chadmiller
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh man. I had not peeked in view.sql before. That is the most upside-down, CTE-abusing SQL I have ever seen.

@chadmiller
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a try at simplifying and refactoring that, if no one else does.

@sebbacon
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made an issue for it: #121

@NickCEBM
Copy link
Contributor

@NickCEBM NickCEBM commented on ce0c22d Mar 13, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take full responsibility for that mess of SQL.

One note, an update to that SQL is coming down the pike soon (see my pull request #116 ) with some new fields. I don't think it looks much prettier but for the 2nd query it is at least commented out and grouped so someone can understand what is going on better and what the various fields represent.

Still probably needs work to make sensible but I think it's a bit easier to follow.

The first query with the json_extracts is just a hulking mess though.

@chadmiller
Copy link
Contributor

@chadmiller chadmiller commented on ce0c22d Mar 13, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickCEBM
Copy link
Contributor

@NickCEBM NickCEBM commented on ce0c22d Mar 13, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll comment that out in my PR. Basically it's pulling in a deprecated data field from a historical dataset that we are using as an exclusionary criteria.

Details here.

Please sign in to comment.