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

refactor: preview_auto to use enum and different option #1991

Merged
merged 2 commits into from
May 1, 2024

Conversation

tessus
Copy link
Contributor

@tessus tessus commented Apr 30, 2024

This is a follow-up PR to #1804. It implements an enum and adds a new table [preview] (should be used for max_preview_height and show_preview when the new settings are implemented, although the keys should be renamed then) and the show_preview_auto option has been renamed as mentioned in #991 (comment)

[preview]
strategy = auto (default) | static (max_of_all_cmds)   # was show_preview_auto

However, I am contemplating whether to change the naming:

strategy -> mode
auto -> dynamic

I believe

  • mode is used in other settings and thus it might be more consistent.
  • dynamic as it determines the height dynamically. auto also fits, so I really have no clue which one is better.

@ellie what do you think?

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

thank you!

@ellie
Copy link
Member

ellie commented May 1, 2024

I think mode vs strategy is a bit ambiguous as while we use mode elsewhere, it's not quite the same thing (search mode, filter mode, etc are all for searching history and aren't "smart" decision makers)

Auto seems fine to me

Happy with this as-is

@ellie ellie merged commit 831dd78 into atuinsh:main May 1, 2024
17 checks passed
@tessus
Copy link
Contributor Author

tessus commented May 1, 2024

Great. I'll update the docs accordingly later tonight.

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