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

Allow sesman-restart-session to be optional? #18

Closed
sogaiu opened this issue Sep 7, 2019 · 6 comments
Closed

Allow sesman-restart-session to be optional? #18

sogaiu opened this issue Sep 7, 2019 · 6 comments

Comments

@sogaiu
Copy link
Contributor

sogaiu commented Sep 7, 2019

First, thanks for writing and maintaining sesman, it seems like a good fit for a number of repl-using setups.

I've been trying to integrate it into such a setup and have had some degree of success by implementing:

  • sesman-start-session
  • sesman-quit-session
  • sesman-more-relevant-p

and calling:

  • sesman-add-object
  • sesman-remove-object
  • sesman-install-menu

at various relevant-seeming places.

I don't particularly feel the need for sesman-restart-session, but sesman-install-menu leads to it always showing the option to invoke it (though it is greyed out before there are sessions I see).

Would you mind elaborating on your rationale for requiring sesman-restart-session (at least that's the impression I got from the README)?

I see that there is a default implementation, but at this point, I think I'd prefer not to give the impression that somehow there is something being preserved.

If it isn't a problem, would it be possible to have sesman-install-menu take an optional argument to supply a value differing from sesman-menu? This way I can provide a different menu and I can implement sesman-restart-session to return nil.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 7, 2019

Restarting a session seems like a pretty common thing to do. Why don't you think it's not particularly useful?

I think that if you don't implement something the simplest approach is to just throw some user-error saying that some functionality is not supported. I wouldn't resort to dynamic menu generation.

@sogaiu
Copy link
Contributor Author

sogaiu commented Sep 7, 2019

As to the question asked, I haven't yet found myself wanting to?

As to the second point:

  • Not sure what you mean by dynamic menu generation. Perhaps you could elaborate on that. May be you could also touch on why you woudn't resort to it.

  • Since in some use cases of sesman, one may have custom context objects [1] [2] and these will not necessarily be the same between two systems that are using sesman, won't it be helpful for the menu to reflect the differences? Doesn't it make sense for the sesman menu that shows up for CIDER (I presume it does?) to be different from the one that shows up for a different system if they don't have exactly the same set of context objects? May be I don't quite understand how it works and what I'm suggesting isn't feasible.

  • I'd rather not clutter the UI with something that I know isn't going to work (or work properly) anyway. I find that misleading as a user and when possible, would not like to subject others to that.

[1] From the README:

By default there are three types of contexts - buffer, directory and project, but systems can define their own specialized context types..

[2] From #15:

It basically comes down to writing custom methods sesman-context and sesman-relevant-context-p.

@vspinu
Copy link
Owner

vspinu commented Sep 7, 2019

@sogaiu what is your use case more concretely? I find it surprising that restart is not needed. Whatever system I can think of it would occasionally require a clean restart. Glitches, stalls, clean-start are common reasons you would want to restart the system. Unless there is a good reason not to have a restart for some use cases I would rather not complicate the current setup with such special cases.

@sogaiu
Copy link
Contributor Author

sogaiu commented Sep 7, 2019

@vspinu I am adding / enhancing session capabilities to a custom repl-based system. I've been using it in various forms for some months, fortunately without much difficulty.

I can see that we're differring in what we expect out of restarting here. I think because there is a notion of session being entertained, I expect more out of restart. If I want a clean start, I am not thinking of it as a restart -- at least when I have a session in mind.

So far, I don't consider the trade-off of writing and maintaining the code to support a good session-aware restart to be worthwhile (e.g. in Clojure terms, restoring what namespace a particular connection had current before restarting, or only reconnecting certain connections that are problematic, etc.). Just quitting and starting again manually seems cleaner -- but I would probably find it misleading to label that restarting in this context.

I can appreciate not wanting to complicate one's code -- I'm saying the same type of thing, just for a different code base :)

On a related note, I'm curious if you have ideas about how to support custom context objects in one's menus so each system can have just what's relevant for their own case. Is that something that seems worthwhile and/or achievable?

@vspinu
Copy link
Owner

vspinu commented Sep 8, 2019

I expect more out of restart.

This is a valid point, but it's out of scope of sesman (I think). Each individual system could provide multiple types of restart (from clean, equivalent to quit+start, to more complex types up to full session restoration). System's designer should deside on the meaningful default and let the user customize that default restart level. The backend could even ask the user interactively for what kind of restart is desired.

FWIW, I would like if CIDER would re-install the current namespace in the repl by default, but not a biggy by all means.

On a related note, I'm curious if you have ideas about how to support custom
context objects in one's menus so each system can have just what's relevant for
their own case. Is that something that seems worthwhile and/or achievable?

This is surely worthwhile. Some contexts would make little sense for some systems.

But this is easy, no? Each system returns a list of context types which it understands in sesman-context-types. One can use that list to refine the menu. Is this what you mean?

@sogaiu
Copy link
Contributor Author

sogaiu commented Sep 8, 2019

Thanks for your detailed comments, they were helpful.

Regarding supporting custom context objects in menus and the matter of customizing the menu:

I misunderstood that it might not be possible to refine the menu with the current code base and didn't explain that very well in the original post. However, with your explanation I think I have succeeded.

I'm also glad to hear that custom context types can thus be supported in the menu, that seems much better from a user experience perspective.

@sogaiu sogaiu closed this as completed Sep 9, 2019
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

No branches or pull requests

3 participants