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.

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

Add basic python client #408

Merged
merged 4 commits into from
Sep 17, 2022
Merged

Add basic python client #408

merged 4 commits into from
Sep 17, 2022

Conversation

NickCellino
Copy link
Contributor

@NickCellino NickCellino commented Sep 6, 2022

Hey there!

This is my first pass at making a very basic python client for Conjure. I followed the Julia example pretty closely and made tweaks where appropriate, but it very much follows the same pattern.

This will probably need a fair bit of work to make it a robust solution for more people, but for my current use-case (building small one-off scripts for LeetCode challenges), it's working great so far! Most fun I've had writing Python code in a while.

I'm not sure if all the minor issues I listed below would be pre-requisites for merging this or if this can be useful for others before those are fully addressed. Either way, I thought I'd put this up and get some feedback on this.

What is working

With what I have here, I can:

  • Evaluate selection
  • Evaluate current form
  • Evaluate root form
  • Evaluate file

Pretty much everything I've tested seems to be working at the moment

Here's a little demo:

https://asciinema.org/a/520309
asciicast

Things I still want to fix

  • It doesn't like empty lines in function definitions because it thinks your function definition has ended before it actually has, like so:
    def for_loop_spacing():
        for i in range(10):
            print(i)
    
        return 4
    • I think removing any blank lines before sending to the REPL should fix this. I just need to actually do that and then make sure there aren't any corner cases there
    • [UPDATE]: I ended up addressing this using treesitter. See the comments in the code for more details but the basic idea is we use treesitter to determine if a section of code is a simple expression and if it is NOT, we turn the code into a multiline string and send it to the python exec function

More minor issues

  • I have not yet implemented doc-str. I haven't looked into what is necessary to implement that, it may be really easy
  • The "ghost text" that appears next in the buffer next to what you are evaluated is not parsed/formatted nicely yet (see image below). If anyone can point me in the direction here, I'd love to fix this. Just haven't had time yet.

Screen Shot 2022-09-11 at 4 19 34 PM

  • I also haven't looked into tests yet. I have a bunch of little pure, utility functions that would probably good to add some tests for

@NickCellino
Copy link
Contributor Author

NickCellino commented Sep 6, 2022

I think removing any blank lines before sending to the REPL should fix this. I just need to actually do that and then make sure there aren't any corner cases there

this seems like a bigger issue than I originally thought. Removing blank lines obviously can screw up multiline strings, but also, the python interpreter wants multiple lines between things like for loops and def statements so removing blank lines would break these things. I think treesitter may help here to give more context about what is being evaluated... I will have to think about this

@NickCellino
Copy link
Contributor Author

I've spent a bit of time playing around with the issue I described above and I think it may be a dead-end or at least a rabbit hole trying to get this to work with the basic python -i REPL. I'm going to explore using ipython instead of python. There just doesn't seem to be any reliable way to "paste" an arbitrary block of code into the python interpreter (see here for some discussion: https://stackoverflow.com/questions/2501208/copying-and-pasting-code-into-the-python-interpreter)

This repo https://github.com/geg2102/nvim-python-repl has provided me with some inspiration for using ipython.

; sending a command is the result of the command.
; Relevant docs: https://docs.python.org/3/library/sys.html#sys.displayhook
(def update-python-displayhook
(.. "import sys\n"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐼 look at str.join for multi line strings like this. Fennel also supports multi line strings out of the box, but with python indent it might get a bit messy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. also throws errors if it encounters a nil, str.join does not, it skips over it just like the Clojure implementation it's copied from. join also pretty prints any non-string Lua data structures it encounters on the way.

I use str.join everywhere instead of .. unless I know I'm joining two simple strings and I KNOW they can not be anything else (i.e. they're not user input or from some other source).

I'll give the rest of the code a review soon :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! If it wasn't clear, I do not think this PR is ready yet. Do you prefer to leave the PR open for discussion or keep the discussion in Discord until this is in a more complete state?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay! No problem, just ping me when you're happy 😄 and either is totally fine. Can always have more back and forth in discord then go here for more review-like comments. Up to you! Whatever UX you prefer.

Copy link

@Dotrar Dotrar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I worked on the Common lisp client. If you need a hand with anything let me know, as I use python for my day job and would love to see this implemented!

If you haven't seen it already, feel free to join the discord! it could be nice to talk to other devs about it. There's no python channel yet but feel free to chat in the common-lisp channel. :D
https://discord.gg/5JC28a67

@NickCellino
Copy link
Contributor Author

Hey @Dotrar! Yeah, it would be great to have more eyes on this and get some feedback. I'll hop on the discord

@NickCellino
Copy link
Contributor Author

NickCellino commented Sep 11, 2022

@Olical @Dotrar I think I'm ready for someone to take a look at this when you guys have some time.

I updated the asciinema recording in the PR description if you want to take a look at how it's working.

I ended up sticking with plain old python3 as the repl instead of switching to ipython.

A couple quirks about this implementation are:

  1. It requires treesitter for the reasons I outline in the PR description as well as in comments in the code in some more depth (should I put those comments somewhere else? In the wiki?). I log a warning and do not start the REPL if the user does not have the python treesitter parser installed
  2. I added a new parameter to the stdio remote client as a workaround for an issue that I found during testing. I mentioned this in the python Discord channel. The issue was that sometimes (when printing something in the body of a for loop is one example), the python REPL would send the next prompt ">>>" (on stderr) before it would send the output of the command that was sent (on stdout). This obviously confused the stdio client and the result never got printed. The new option allows you to add a delay to the stderr callback. It's not pretty, but it works. If anybody has ideas for a more elegant solution here, I'm all ears.

@NickCellino
Copy link
Contributor Author

Small update: I implemented doc-str and fixed the ghost text/virtual lines:

https://asciinema.org/a/520939
asciicast

@Olical Olical changed the base branch from master to develop September 17, 2022 12:48
@Olical
Copy link
Owner

Olical commented Sep 17, 2022

Ohh I like the warning you added if you don't have TS set up.

@Olical
Copy link
Owner

Olical commented Sep 17, 2022

It works great! Nice one, everything "just worked" as expected. Code looks okay to me too, nothing stands out to me. I'm gonna just LGTM this and iterate on it if we ever need to.

The whole point in isolating all client code into it's own modules / dirs allows us to just throw code in anyway since it won't interfere with other clients. So we can just try stuff out and iterate safely since it's only loaded if people try to use it.

Thanks a lot! Now to sort out docs changes...

@Olical Olical merged commit 7d7b12c into Olical:develop Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants