xmonad-contrib icon indicating copy to clipboard operation
xmonad-contrib copied to clipboard

X.P: Add escape hatch for preventing X.P IO

Open dcousens opened this issue 1 year ago • 9 comments

Rationale

When using XMonad.Prompt for multiple (8+) different use cases in my configuration, I never once needed a command history.

The default of reading and writing a file for the command history opaque to the user is strange to me, and each prompt is sharing and replacing the same prompt-history file... but I digress.

My feature request is to stop that behavior, where historySize = 0 didn't cut the mustard since prompt-history is still reading and writing on every prompt.

Proposal

This pull request adds historyRead and historyWrite configuration options to XPConfig, allowing users to write something like:

  ...
  , historyRead = \cacheDir -> return empty
  , historyWrite = \history cacheDir -> return ()
  ...

This could open up functionality for using other state for a command history, but maybe that isn't desired. My main priority with this pull request is to prevent the reading and writing of the prompt-history file, and as such I am marking this as a draft until discussion resolves the best path forward.

Alternative proposal (not implemented)

historySize = 0 could branch the behavior, and prevent any related IO (reading or writing).

Checklist

  • [x] I've read CONTRIBUTING.md

  • [x] I've considered how to best test these changes (property, unit, manually, ...) and concluded: unknown, I am using these changes locally

  • [ ] I updated the CHANGES.md file

dcousens avatar Jan 18 '24 00:01 dcousens

Maybe a better path forward is to remove the prompt-history writing behaviour by default in a breaking change, and then supporting users to opt-in by adding this kind of configuration?

  ...
  , historyRead = readPromptHistory "prompt-history"
  , historyWrite = writePromptHistory "prompt-history"
  ...

dcousens avatar Jan 18 '24 00:01 dcousens

@slotThe historyCompletionP needs conf either way, but yes branching as I mentioned is an alternative approach

historySize = 0 could branch the behavior, and prevent any related IO (reading or writing).

I'm happy to implement that if the alternative isn't preferable

dcousens avatar Jan 19 '24 05:01 dcousens

@slotThe I have changed the pull request to pattern match historySize = 0 for readHistory and writeHistory

dcousens avatar Jan 19 '24 05:01 dcousens

@slotThe the only downside to a non-breaking change is that historyCompletionP, if used, will not respect the new behaviour (for better or worse), I think this is the right path for now though

dcousens avatar Jan 19 '24 23:01 dcousens

Both historyCompletion and historyCompletionP aren't very widely used within contrib, so I would even be open for a breaking change there, perhaps making them take an XPConfig (or even just the historySize) as an additional argument. I think the number of (actually useful) custom prompts using these in the wild is relatively low

slotThe avatar Jan 20 '24 05:01 slotThe

@dcousens ping for ^

slotThe avatar Feb 09 '24 08:02 slotThe

@slotThe to clarify, you think I should introduce a breaking change [that you have suggested] in this pull request? Assuming yes, I'll try and do that this weekend

dcousens avatar Feb 09 '24 12:02 dcousens

@dcousens sorry for only getting back to you now: I'm saying that I feel ambivalent about introducing a breaking change for historyCompletion[P]; these functions probably aren't very widely used, and the uses they have inside of contrib are all in the vicinity of an XPConfig argument. However, if you feel otherwise them I'm also fine with merging this as-is

slotThe avatar Feb 12 '24 06:02 slotThe

OK, I'll have a shot at that, I'd prefer that personally

dcousens avatar Feb 12 '24 12:02 dcousens

@dcousens friendly ping :)

slotThe avatar Mar 23 '24 06:03 slotThe

Rebased and added a breaking commit, which ensures that historyCompletion* now accepts an XPConfig

dcousens avatar Mar 23 '24 12:03 dcousens

Thank you!

slotThe avatar Mar 31 '24 07:03 slotThe