ResidentMario / py_d3

D3 block magic for Jupyter notebook.
MIT License
451 stars 40 forks source link

Local load possibility and automatization of lastest remote releases added #11

Closed mondeja closed 6 years ago

mondeja commented 6 years ago

You can load local versions of the library with %%d3 local="d3.v4.min.js" (fixes #5) and the latest release stored at https://cdnjs.com/libraries/d3 is selected automatically via cdnjs API, so you don't need to hardcode them every new version. Also, I've added an argument releases that can be called as line magic (simply %d3) or cell magic (%%d3 releases) that show all available releases stored at https://cdnjs.com/libraries/d3.

ResidentMario commented 6 years ago

Very cool! :smile: I'll hopefully take a look at this on the weekend.

mondeja commented 6 years ago

Any inconvenient, let me know.

mondeja commented 6 years ago

Sorry, but the code is growing a lot (now over 500 lines of code). in this new commit there are many improvements. If you don't want these changes, I will fork and continue supporting myself, I understand it.

ResidentMario commented 6 years ago

This is looking like a very big PR. I'll hopefully assess things this weekend, but note that it will take me some time to chew through it. =)

ResidentMario commented 6 years ago

OK so I started looking at this again. You're right, it's a lot of lines, very different from what I remember writing. Not a bad thing exactly, but it's difficult for me to review because of that.

Your code looks fundamentally good. Good style, good use of annotations, a few minor linting issues but nothing major. Your tests all pass on my machine.

Can you pontificate in a comment on what features that you've added in this PR? I'd like to test each of them myself in isolation. =)

mondeja commented 6 years ago

Of course:

Code generator commands (%d3 generator and %d3 gist) needs more testing because some are not working yet. For example, these needing external D3 dependences like d3-geo. We need a more complex external dependencies injection (I'm working on this).

You can see a complete documentation in doc/ folder (run online here). Any questions make me known.

ResidentMario commented 6 years ago

Right, so this PR is too large to cover all at once. Let's break it up into several steps.

  1. Can you submit a PR with just the local version library, online documentation, and verbose output features? I think I can merge that right away.
  2. Let's set up a documentation microsite (like e.g. this one). The feature surface is going to be large after this set of PRs is done that users are going to need it. =)
  3. Once that's done we can work on a separate PR for the online examples and GitHub gists, and adding that to the documentation.

Looking forward to 0.3.0!

ResidentMario commented 6 years ago

I've merged that first PR.

I've updated the documentation (and shed a good deal of weight off of it) to fit the expanded feature surface. I've had a think, and I think that the remaining features can also be handled as further additional sections to README.md, with Binder badge-links sprinkled in.

Can you re-base the remaining additional features (online examples, gists) and the test suite onto the current master in a separate PR?

The notebooks we can sort out later. I suspect this is a good time to come up with a new structure for that stuff.

ResidentMario commented 6 years ago

Besides the notebooks (which aren't too important), are there any other features that still need to be PR'd?