Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ISSUE-72: datafy/nav extensibility for arbitrary Python objects #73

Merged
merged 15 commits into from
Feb 1, 2020
Merged

ISSUE-72: datafy/nav extensibility for arbitrary Python objects #73

merged 15 commits into from
Feb 1, 2020

Conversation

jjtolton
Copy link
Contributor

@jjtolton jjtolton commented Jan 31, 2020 •
edited
Loading

Closes: #72

e.g.

(require '[libpython-clj.python :refer [py..]])
(require '[libpython-clj.require :refer [require-python pydafy]])
(require '[clojure.datafy :refer [datafy]])
(require-python 'requests)

(defmethod pydafy 'requests.models.Response [r]
  (py.. r -content (decode "latin-1")))

(-> "https://www.google.com" requests/get datafy) ;;=> <html>...

@jjtolton
Copy link
Contributor Author

jjtolton commented Jan 31, 2020 •
edited
Loading

I'm not crazy about having to resort to strings for this dispatch but not sure how to do this dynamically otherwise!
EDIT: changed to symbols. Way classier!

@jjtolton
Copy link
Contributor Author

jjtolton commented Jan 31, 2020 •
edited
Loading

@cnuernber let me know if there are other types I should include. I got PPyObject and PBridgeToJVM, and that seems to work for most cases.
https://github.com/cnuernber/libpython-clj/pull/73/files#diff-34d04f0ccd8b38700867cc2740ec63d2R377

@jjtolton
Copy link
Contributor Author

jjtolton commented Jan 31, 2020 •
edited
Loading

Also pydafy might be a stupid name for the multimethod. Open to suggestions.

@cnuernber
Copy link
Collaborator

This looks good but I am concerned the nav support isn't perfect (which honestly isn't a blocker - one step at a time).

Have you tried this in the rebel data viewer?

It is a bit hypocritical for me to ask this since I didn't test the metadata pathway in Rebel....

Aside from the above, do you feel this PR is ready to land? Maybe we can file another issue about Rebl and leave it there, thus landing this and moving a bit forward.

@jjtolton
Copy link
Contributor Author

jjtolton commented Feb 1, 2020

This may have been a lack of imagination on my part. I'm not sure what is worth nav'ing in the standard library. I'll look into it

@jjtolton
Copy link
Contributor Author

jjtolton commented Feb 1, 2020 •
edited
Loading

Tested that datafy/nav seems to work with modules on REBL. I'm not an expert on REBL so perhaps we can make a seperate issue for testing that. Recommend merging this and creating separate issue for REBL testing.

@cnuernber cnuernber merged commit 7ead006 into clj-python:master Feb 1, 2020
@jjtolton jjtolton deleted the ISSUE-72-datafy-nav branch February 1, 2020 23:07
@cnuernber cnuernber mentioned this pull request Feb 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make datafy/nav user extensible for arbitrary Python objects
2 participants
@jjtolton @cnuernber