NetLogo / Tortoise

Compiler and runtime engine for NetLogo models that runs in JavaScript 🐢
https://netlogoweb.org
Other
56 stars 27 forks source link

Add support for Fetch and Import-A extensions, and much more #205

Closed TheBizzle closed 5 years ago

TheBizzle commented 5 years ago

Lord Jeremy, save me, I'm ready for this project to be over.

Here's my summary of what's in this PR:

TheBizzle commented 5 years ago

The "branch" tests failed, because (I think) the "pr-merge" tests and the "branch" ones interfered with one another during the import/export portion of TestModels. I reran the "pr-merge" tests afterwards, and they passed, so we can consider all of the tests passed.

LaCuneta commented 5 years ago

This looks awesome! I'm a big fan of pulling out the polyfill code into its own home, and of course of letting people get data in and out of their models in a sensible way. I looked through the code and nothing caught my attention.

I pulled the branch down and tried to run it locally and hit a problem. For the Import-A command tests: Fetch-Integration, ImportPcolors_2D, ImportPcolorsTopologyTest_2D, and RoundTripWithUTF8Chars threw a null reference exception in slurpByType() because the mimeType was null from the call to Files.probeContentType(). I did some brief checking and it looks like Files.probeContentType() might not be reliable across platforms. The other solutions listed there also don't seem great. I tried letting mimeType == null through to be slurped as text, and that failed as well, probably because it wasn't really text. I don't have a great solution, since the alternatives seem like a lot of effort for such a small thing that would only be used in development and testing.

Other than that, I think this is good to go.

TheBizzle commented 5 years ago

:sob:

TheBizzle commented 5 years ago

Alright, I've just pushed a fix to the branch. Everything should (hopefully) be in order now. For maximum compatibility, uninstall your old version of the desktop edition of the Fetch extension. (Deleting your ~/.netlogo folder is a quick and easy way to do this.)

LaCuneta commented 5 years ago

I confirmed the change has things working on my machine now. Waiting for the tests to pass and then I'll merge.