search

sfu-natlang / lensingwikipedia

Lensing Wikipedia is an interface to visually browse through human history as represented in Wikipedia. This the source code that runs the website:
http://lensingwikipedia.cs.sfu.ca
Other
11 stars 4 forks source link

Queries API refactoring #217

Closed theq629 closed 7 years ago

theq629 commented 7 years ago

My proposal for #190 and #189, which also fixes #10. Also a fix for #215.

Note that this touches a lot of code, and see concerns on #189.

I know that guidelines are against having a merge commit in here, but it was a very complicated merge so it seemed better to do it that way.

anoopsarkar commented 7 years ago

@avacariu can you do a code review for this pull request?

theq629 commented 7 years ago

I would suggest a code review plus testing of the logging features (which I wasn't familiar enough with to do properly).

avacariu commented 7 years ago

Sorry for the late reply; somehow I only saw the notification for the other PR which was closed. There are way too many lines changed for any useful code review, so I only have a couple points.

One thing I noticed was here: https://github.com/sfu-natlang/lensingwikipedia/pull/217/files#diff-07941c07191fd1ef5c8f9fc3d23b211dR21 cnstrI is never going to be 0

Related to that area of code: how come the JSON is created manually instead of using JSON.stringify? It seems like it's so that the JSON's keys are sorted, but JSON isn't ordered by design, and neither are python's dicts. I noticed JSON created like this in a couple other places, and it seems very error-prone.

In terms of logging, the things that were logged were very specific to the user study we ran, and I think it's fine to change as features change. Some of the things were being logged in very hacky ways anyway.

theq629 commented 7 years ago

One thing I noticed was here: https://github.com/sfu-natlang/lensingwikipedia/pull/217/files#diff-07941c07191fd1ef5c8f9fc3d23b211dR21 cnstrI is never going to be 0

Thanks, I'll change that.

Related to that area of code: how come the JSON is created manually instead of using JSON.stringify? It seems like it's so that the JSON's keys are sorted, but JSON isn't ordered by design, and neither are python's dicts. I noticed JSON created like this in a couple other places, and it seems very error-prone.

It's so the per-constraint JSON snippets can be to check uniqueness before building the final structure to send to the backend. I couldn't come up with a cleaner way to merge pre-stringified snippets short of re-stringifying.

In terms of logging, the things that were logged were very specific to the user study we ran, and I think it's fine to change as features change. Some of the things were being logged in very hacky ways anyway.

Ok, in that case the logging code here should be fine for now.

theq629 commented 7 years ago

I removed the unused cnstrI code.

avacariu commented 7 years ago

Great, I'll merge this now and if there are any other issues, we can deal with them later.

anoopsarkar commented 7 years ago

@avacariu can we deploy this version for testing? Maybe on a new port and leave the current version running at the same time. Orland wants to keep the version we used for the user study around for a follow-up user study.

avacariu commented 7 years ago

Yeah, I'll set it up.