elixir icon indicating copy to clipboard operation
elixir copied to clipboard

Allow IEx to accept a dynamic prompt

Open donquxiote opened this issue 2 years ago • 8 comments

Allows for an anonymous function be passed to the IEx prompt configuration. This function takes a map of the prompt strings so that the user can add them where they want. The original method of a string configuration is preserved.

Adds a test and documentation for this functionality.

donquxiote avatar Jul 28 '22 20:07 donquxiote

Hi @donquxiote, thanks for the PR! However I have some concerns:

  1. Anonymous function is not allowed in config files, so this would not be possible if you are configuring IEx in your app config

  2. Having a function means you can completely break the prompt and render IEx non-functional

  3. In case of remote shells, we will need to document that the function either runs on the remote shell or the local one. Running in the local one may be more efficient, but you won't have access to modules from the actual application, which may be misleading depending on the use cases. Whatever choice we make will also make some changes in IEx not possible.

So IMO there are too many pitfalls here. Unless there is a very strong use case for this feature, I don't think we should support it.

josevalim avatar Jul 29 '22 06:07 josevalim

I really miss this feature from Ruby's IRB.

We have a multi-tenant app and it's really nice to be able to see what tenant you're currently set to right in the prompt.

cjbottaro avatar Jul 29 '22 14:07 cjbottaro

@cjbottaro aren't you able to do so with the static prompts?

josevalim avatar Jul 29 '22 14:07 josevalim

Not that I'm aware of. Our multi-tenant system works like this:

Tenant.set "foo"
Tenant.get
"foo"

And how I imagine the prompt to be...

iex> Tenant.get
nil

iex> Tenant.set "foo"
"foo"

iex:foo> Tenant.set "bar"
"bar"

iex:bar> 

I thought the static prompt only interpolates a few known place holders like %counter and %prefix and such.

cjbottaro avatar Jul 29 '22 17:07 cjbottaro

@josevalim Those are good points. @cjbottaro and I are on the same team, so that multi-tenant app is my use case I was trying to solve for.

On your other points;

  1. What if the function is passed as an MFA instead of the anonymous function? (latest commit)

  2. That occurred to me, but if the documentation is clear, in my opinion the onus would be on the user to ensure their function is ok. That being said, I'm not a maintainer or a forum mod, and I realize there's a possibility that y'all would have to wade through false bug reports if people break IEx and don't check their function first.

  3. This sounds tricky for sure. I haven't used IEx as a remote console before, but my first thought is it should execute on the remote. When I'm running IEx locally, I expect it to use the files/configs on my machine in that repo, if I'm opening a remote shell I'd expect everything to running from that live machine. What changes would this prevent that you're thinking of?

donquxiote avatar Jul 29 '22 20:07 donquxiote

@donquxiote @cjbottaro and in your case, where is the tenant stored? If you store it in the process dictionary, for example, is it guarnateed that we ask for the prompt in the same process that evaluates code?

If the tenant is global... then you will affect the tentant used by the whole system, and it is generally a very bad idea.

In any case, MFA definitely solves the first problem, but I don't think there are solutions to problems 2 and 3. About 3, I am thinking exactly that we are making some assumptions about where those functions are evaluated, and I am not sure any of those assumptions are good or should be make permanent (is it local? remote? in the evaluator process? or elsewhere?).

josevalim avatar Jul 29 '22 20:07 josevalim

Yes, the tenant is stored in process dictionary, and also using $ancestors and $callers so that "sub" processes (usually Tasks and/or GenServers) can identify which tenant is set.

In our use case, we start up iex, set a tenant, run some Ecto queries and functions, switch to a different tenant, then run some more queries and what not. So yes, the tenant is set in kind of the "root" process.

Would it be enough to default running the prompt locally, but allowing it be configured to run remote? Tbh, I am completely unfamiliar with remote IEx. I'm kind of assuming that if you start up iex telling it connect to a remote node, then the iex process is running on the remote node and thus running Tenant.set "foo" happens on the remote node and if the prompt is evaluating locally, then Tenant.get will just always be nil. Yeah, that seems problematic... :|

cjbottaro avatar Jul 30 '22 17:07 cjbottaro

So I checked the patch locally and the current MFA/function does not run in the context of the evaluator, which means the current approach is not enough for you to get the tenant feature you desire.

I would recommend exploring/solving the problem in a way that it would work for your needs. I am not exactly sure what that would be, but maybe a different solution avoid the pitfalls mentioned above. Maybe we can have some sort of evaluation context, that runs in the evaluator after each evaluation, and we send that to the server as a string and include it in the static prompt.

But the important is to check that it indeed works for your use cases. :)

josevalim avatar Jul 31 '22 07:07 josevalim

@josevalim I concur with you!

avosa avatar Aug 20 '22 12:08 avosa

I will close this for now based on the above. Thank you!

josevalim avatar Aug 20 '22 13:08 josevalim