Skip to content

feat: pure_12hr_system_time option for 12 hour system time#412

Open
ThatOneCalculator wants to merge 2 commits intopure-fish:masterfrom
ThatOneCalculator:feat/12-hour-systime
Open

feat: pure_12hr_system_time option for 12 hour system time#412
ThatOneCalculator wants to merge 2 commits intopure-fish:masterfrom
ThatOneCalculator:feat/12-hour-systime

Conversation

@ThatOneCalculator
Copy link

Specs

12 hour system time, if system time is enabled

New config in conf.d/pure.fish

_pure_set_default pure_12hr_system_time false

Documentation

Usage

set --universal pure_12hr_system_time true

Acceptance Checks

  • Documentation is up-to-date:
    • Add entry in feature list of README.md ;
    • Add entry in features' overview in docs/ ;
    • Add section in feature list to document
      • Features' flag ;
      • Prompt symbol ;
  • Default are defined in conf.d/pure.fish for:
    • Feature flag ;
    • Symbol ;
  • Tests are passing (we can help you 🤗 ):
  • Customization is available ;
  • Feature is implemented.

Copy link
Member

@edouard-lopez edouard-lopez left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
issue: the implementation is far too specific, and we don't want to re-implement every date-supported format.

suggestion: A simpler solution would be to

  1. set the format in a universal variable (cf. conf.d/pure.fish) like pure_show_system_time_format ;

  2. set its default to +%T (so we preserve existing behaviour) ;

  3. then users can simply override this variable on their config with

    set --universal pure_show_system_time_format '+%I:%M:%S %p'
    

todo: add a sample in the doc

@ThatOneCalculator
Copy link
Author

Done! Made the variable pure_system_time_format since that reads better imo, I could change it if you want though

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