michael-maltsev / cheese-fork

A scheduling helper web application for Technion students
https://cheesefork.cf
GNU General Public License v3.0
50 stars 13 forks source link

Data-compatibility with ttime3 #5

Closed lutzky closed 6 years ago

lutzky commented 6 years ago

Yo, @lutzky here from http://github.com/lutzky/ttime3! Awesome project you have here. While I think the two frontends are worth keeping separate, it may be helpful to join forces on the data format.

For ttime to be able to load a catalog file, it needs to be:

Thankfully, that last one is already granted by virtue of being hosted on a github pages branch:

# also works when replacing gh-pages below with a specific commit ID
$ curl -s -D - "https://raw.githubusercontent.com/michael-maltsev/cheese-fork/gh-pages/courses_201702.js" -o /dev/null | grep -i access-control
Access-Control-Allow-Origin: *

The remaining work is JSON and the specific format. Currently, the files courses_YYYYMM.js are not JSON because of the var courses_from_rishum = prefix, which is required because of the way you load the data:

https://github.com/michael-maltsev/cheese-fork/blob/3b0896cb498f308fd65e01d0477c52061948a66e/deploy/index.html#L106-L107

However, if you drop that prefix, you can load the data in this way, which I believe is more idiomatic:

let req = new XMLHttpRequest();
let result = null;
req.open('GET', url);
req.onload() = function() {
  if (req.status == 200) {
    result = JSON.parse(req.response);
  }
}

In ttime3: https://github.com/lutzky/ttime3/blob/ec742348bd2062a4ae2a87c8c2c3297428de7c6b/common.js#L62-L72

I can send you a pull request for this, if we're not concerned about breaking backwards compatibility with anything.

Finally, the structure of the data itself is a bit different. For one, you use Hebrew field names, which I find to be a bit unwieldy inside the code. More importantly though, the courses are not separated into faculties (instead they include the faculty name) and the fieldset is a bit different; instead of dividing into groups, there are the fields קבוצה and מס., which I'm not sure how to reason about. The TTime/Repy data structure is defined in https://github.com/lutzky/repy/blob/master/datastruct.go. I think the best thing to do for compatibility here would be to add a conversion module, in Javascript, to ttime itself - which I can do once I understand the cheesefork data structure.

What do you think?

michael-maltsev commented 6 years ago

Hi!

if you drop that prefix, you can load the data in this way, which I believe is more idiomatic:

I considered that, but that doesn't work if CheeseFork is opened locally, at least not in Chrome. See here. So I use what can be called a sloppy JSONP, defining a global variable instead of calling a function. I can consider converting it to proper JSONP, but I prefer not to use AJAX due to the mentioned limitation.

For one, you use Hebrew field names, which I find to be a bit unwieldy inside the code.

This allows the scraping code to be generic, converting the rishum table layout to JSON without being familiar with the actual keys. See this page for example, and compare it to the JSON data, you will see the similarity.

Using the Hebrew keys in the code is arguably not optimal, but you can always define a mapping.

the courses are not separated into faculties (instead they include the faculty name)

The rishum pages don't include a course's faculty. I deduce it from the course number. See here.

instead of dividing into groups, there are the fields קבוצה and מס., which I'm not sure how to reason about

This can indeed be confusing. I suggest to look at some rishum pages, where you can see a more friendly table layout. Once you realize the logic, and the fact that the JSON contains exactly the same information, I guess you'll be able to implement the proper conversion.

lutzky commented 6 years ago

I see what you mean. ttime3 does not work locally at all for this reason (but I just run python -m http.SimpleHTTPServer to get around that). That's your decision, of course. However, it seems like if you do convert it to proper JSONP, that means courses_YYYYMM.js does becomes valid JSON, so ttime3 can read it using XHR.

Then again, I can just have my converter drop the var courses_from_rishum = prefix. :man_shrugging: It's probably a better idea, and I can make it future-compatible in case you do decide to transition to JSONP.

I understand your reasoning for sticking to the format of the Rishum pages. I'll spend a bit of time wrapping my head around it to implement a proper conversion.

Thanks! I'll close this bug here in favor of https://github.com/lutzky/ttime3/issues/12.

michael-maltsev commented 6 years ago

if you do convert it to proper JSONP, that means courses_YYYYMM.js does becomes valid JSON

Nope, it becomes a different non-JSON file. See e.g. here.

I can just have my converter drop the var courses_from_rishum = prefix. 🤷‍♂️

Or use the same technique as I do, and as a bonus save yourself the python -m http.SimpleHTTPServer calls :) But it's your call, of course.

lutzky commented 6 years ago

Seems like it should become a JSON file, which can either be accessed directly or through a script that would add the wrapping code.

michael-maltsev commented 6 years ago

Since GitHub serves only static files, the only option is to serve a file of the form: mycallback({...});.

lutzky commented 6 years ago

...and you're already doing the moral equivalent of that. However, you do already generate the files, so you could ostensibly generate a .json file without the var courses_from_rishum = prefix, and generate the .js file from that.

I don't think this is worth doing though, as it provides you no benefit and the integration with ttime3 is theoretical at this point. :smile:

lutzky commented 5 years ago

This is now implemented (see http://lutzky.net/ttime3, with Settings->Presets), without needing any changes to cheesefork. Thanks for making this data available!