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

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

Merged
merged 1 commit into from
Mar 31, 2024

Conversation

dcousens
Copy link
Contributor

@dcousens dcousens commented Jan 18, 2024

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.

Proposal (current)

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

Checklist

  • I've read CONTRIBUTING.md

  • 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

XMonad/Prompt.hs Outdated Show resolved Hide resolved
@dcousens

This comment was marked as outdated.

Copy link
Member

@slotThe slotThe left a comment

Choose a reason for hiding this comment

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

Wouldn't the easiest thing be to just not write the history file if the history size is 0?

@dcousens
Copy link
Contributor Author

dcousens commented Jan 19, 2024

@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
Copy link
Contributor Author

dcousens commented Jan 19, 2024

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

Copy link
Member

@slotThe slotThe left a comment

Choose a reason for hiding this comment

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

This looks fine to me, and is definitely the least intrusive change. Neither readHistory, nor writeHistory is exported, so this does not result in a breaking change for anyone.

@dcousens dcousens marked this pull request as ready for review January 19, 2024 13:18
@dcousens
Copy link
Contributor Author

dcousens commented Jan 19, 2024

@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

@slotThe
Copy link
Member

slotThe commented Jan 20, 2024

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
Copy link
Member

slotThe commented Feb 9, 2024

@dcousens ping for ^

@dcousens
Copy link
Contributor Author

dcousens commented Feb 9, 2024

@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

@slotThe
Copy link
Member

slotThe commented Feb 12, 2024

@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

@dcousens
Copy link
Contributor Author

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

@slotThe
Copy link
Member

slotThe commented Mar 23, 2024

@dcousens friendly ping :)

@dcousens
Copy link
Contributor Author

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

This contains a breaking change for readHistory, writeHistory,
historyCompletion, and historyCompletionP to take an XPConfig, so they
are aware of this choice. While the latter two are exported, it seems
unlikely to affect many users.
@slotThe slotThe merged commit 6e43da8 into xmonad:master Mar 31, 2024
18 checks passed
@slotThe
Copy link
Member

slotThe commented Mar 31, 2024

Thank you!

@dcousens dcousens deleted the no-xp-io branch April 1, 2024 15:49
slotThe added a commit to xmonad/xmonad-extras that referenced this pull request Apr 3, 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.

2 participants