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

Make $status available in the interactive loop #66

Closed
wants to merge 2 commits into from
Closed

Conversation

jpco
Copy link
Collaborator

@jpco jpco commented Dec 1, 2023

The creatures feep, but I think this creature is well-justified and expected by the "average" user.

The variable is called $status for cultural compatibility with rc and
direct compatibility with $bqstatus.
@wryun
Copy link
Owner

wryun commented Dec 1, 2023

Given that it's only in the interactive loop, seems mostly harmless, but... let me sit on this for a bit. Given $status is also referenced in the exception handling, there's probably some edge case where the behaviour is different (e.g if you redefine status in fn-dispatch or fn-prompt and then somehow eof), though I don't think it would cause actual harm. I guess a safe option would be to keep the existing $result variable for the exception handling and never use $status.

My preference as a user for this sort of thing is to surface it in the prompt, which I guess this also enables.

@hyphenrf
Copy link

hyphenrf commented Dec 2, 2023

for prior attempts context #33

I agree that the only real value of this is in interactive use, in shell and in the prompt, as there's no "checking it after the fact" in scripts -- can just capture the evaluation result as one normally does.
As a side-effect of the way it was done in that issue linked above, variable bindings were also reflected in $status, meaning

; x = 1
; echo $status
1

I'm not commenting from my personal machine, so I can't test whether it's the case here.

Here's a use-case of $status from my .esrc

# override prompt to take a different color on nonzero
# TODO: x = 1 returns 1; and is thus a "nonzero status"
#
let (ogprom = $prompt)
fn %prompt {
   if { nonzero $status } {
      prompt = <={ rgbcol $ogprom(1) 255 55 55 } $ogprom(2 ...)
   }{
      prompt = $ogprom
   }
}

@jpco
Copy link
Collaborator Author

jpco commented Dec 2, 2023

hyphenrf: I didn't see there was previous context here -- thanks for posting that. I should take a closer look at prior art before throwing ideas around. Nice that we came up with essentially exactly the same change. And yes, you're right about assignments setting $status.

wryun: I think I see what you're talking about? Something like this?

let (pr = $fn-%prompt; fuse = three two one)
fn %prompt {
	if {~ $fuse ()} {
		status = 'Boom!'
		throw eof
	} {
		fuse = $fuse(2 ...)
		$pr $*
	}
}

I guess that's bad, but IMO the main problem there is %prompt having no dedicated exception catcher protecting the interactive loop. For example, I think this is much more unpleasant, and works at HEAD:

let (pr = $fn-%prompt; fuse = three two one)
fn %prompt {
	if {~ $fuse ()} {
		throw error %prompt 'Boom!'
	} {
		fuse = $fuse(2 ...)
		$pr $*
	}
}

@wryun
Copy link
Owner

wryun commented Sep 11, 2024

Looking at this again, the other thing that makes me a bit wary is that it might be confusing to users to have status only in the interactive loop and bqstatus everywhere.

This made me look at $status in rc. Given that es seems to have inherited bqstatus from rc, if we're going to have a status maybe it would be nice to have an rc style status?

e.g.

; false | true | true | false
; echo $status
1 0 0 1

But in case it wasn't already clear, as far as I'm concerned if you're keen enough about something feel free to merge it; active maintainer wins IMO!

@jpco
Copy link
Collaborator Author

jpco commented Sep 11, 2024

I think you have a good point about the asymmetry of it -- that seems like a bummer for people who didn't opt into that asymmetry on purpose. And I'm definitely not interested in adding $status to scripts. I prefer the explicit control flow and, from reading the rc list, it seems like "magic variables" are generally a rich source of confusion.

One thing about es that I'm a little dissatisfied with is the "big lift" of redefining %interactive-loop for little changes. In this case you could probably do a %dispatch hack, but that has its own trickiness to work around es' own resetting of %dispatch.

I think I might close this PR and instead propose documenting this as a caveat in the man page.

@wryun
Copy link
Owner

wryun commented Sep 11, 2024

Hmm, could add a hook function to interactive-loop or dispatch? e.g. if a function %dispatch-result is defined, call it with the result as the argument?

tbh, it still has those problems you identify with explicit control flow and still pollutes the global namespace, but it somehow seems cleaner and more functional to me.

@jpco
Copy link
Collaborator Author

jpco commented Sep 12, 2024

Maybe. Thinking about it, I do consider addressing the flexibility of the REPL to be better than what I've proposed in this PR (generality and extensibility is the es way!) But it's also a much bigger design task.

Personally, the most common things I think about changing in %interactive-loop are:

  • Interactive startup -- Easy with normal function overloading, no need to do anything special

  • %dispatch -- The cleanest way is probably to invoke the Fundamental Theorem: have es muck around with some internal %es-dispatch function, and just set fn %dispatch {$fn-%es-dispatch $*} in initial.es. then users can overload %dispatch to their hearts' content and as long as the new definitions still call %es-dispatch internally (as would happen by default with the usual function-overloading pattern) things should Just Work (TM).

Trying it out -- with a hacked es that does the above, putting the following by itself in .esrc actually does work (and es -x and friends still do too):

let (d = $fn-%dispatch)
fn %dispatch {status = <={$d $*}}

This simple version isn't quite as sophisticated as would be preferred -- no dynamic scoping for status, for example, and doesn't account for %interactive-loop vs. %batch-loop -- but simple hacks have simple behavior, and it's reasonably easy to imagine how to make this more sophisticated. Maybe it would be worth a bit of thought on how to protect %batch-loop by default. Maybe %batch-loop just calls %es-dispatch directly -- that way it's still hackable, but it's harder to do so, and that seems appropriate for %batch-loop, which usually shouldn't be messed with nearly as much as %interactive-loop

  • Exception handlers -- errors, signals, and "other".

This is, to me, the largest open question. These are the least extensible part of %interactive-loop right now -- nestled inside nontrivial, load-bearing control flow, with no hook functions available to help -- and they seem to me to be entirely reasonable to want to extend (for example: on my own machine I have installed a sigwinch handler that maintains the LINES and COLUMNS variables in the environment).

I personally have my %interactive-loop set up to use rc-like fn-sigwinch-style handlers, and they're pretty nice. But for the distributed es that feels like it's imposing too much of a specific pattern. On the other hand, I don't think I'd want to simply change the entire %interactive-loop exception handler to be a big hook function -- that seems like it would make it too easy to break eof while trying to install some at-exit handler.

Thoughts on any of this?


As an aside, here's my favorite way to edit %dispatch to set status in es today. It's certainly tricky (really depends on all those distinctions between local and let!) but I think it's correct, and it's fairly concise at least.

let (il = $fn-%interactive-loop)
fn %interactive-loop {
	let (d = $fn-%dispatch)
	local (
		noexport = $noexport status
		status = <=true
		fn %dispatch {status = <={$d $*}}
	) $il $*
}

@jpco jpco mentioned this pull request Oct 18, 2024
@jpco
Copy link
Collaborator Author

jpco commented Nov 2, 2024

With canonical extensions (#131), this can be closed.

@jpco jpco closed this Nov 2, 2024
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