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

Issue 20 harcoded 12h time format #23

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

brunetton
Copy link
Contributor

Fix #20

self.tag = None

@property
def description(self):
return f"{self.name} at {self.end_time.strftime('%-I:%M %p')}"
return f"{self.name} at {self.end_time.strftime('%-H:%M %p')}"
Copy link
Member

Choose a reason for hiding this comment

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

@brunetton does this break the AM/PM format?
Some people may be using it, I assume

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good question and I searched a while to figure out. At the beginning I thought that using %H with %p would check in locale def for the locale preferences and use 12h or 24h according to it. But the doc isn't clear on this point.

I digged around and I'm not sure anymore: is the 12h format automatic when locale is en_US ? Apparently this is a free interpretation. It made quite a mess in Debian when they changed it (I didn't read the whole thread).

I didn't found any "official" reference on this and this comment seems to go in flavor of local preferences.

I tried using specialized libs like Pendulum and Arrow but they seems to follow the same formatting as standard DateTime: 24h format by default.

So I guess this is a matter of local choice and I suggest that we add an option to the user to use 12h format or not.

What do you think about ?

@gornostal
Copy link
Member

gornostal commented Mar 27, 2023 via email

@brunetton
Copy link
Contributor Author

To be sure to understand: if the user type something like ti 1pm we'd assume that he wants 12-h format, else we use 24h format ?

  • but in case it's 11am and user stars a timer for 2h, how to guess if he prefers to see "timer set for 13h" or "timer set for 1pm" ?
  • and in case the user uses (like me) 100% of the time the countdown functionality: ti 30m tea time how to guess ?

IMHO a global option for 12h format is simple to understand for users and super keep a code simple to maintain. I can do this :)

@gornostal
Copy link
Member

gornostal commented Mar 30, 2023

@brunetton I took a closer look at the code. I realized I may have misunderstood your intentions.
If you just want to add an option to display the time in 24h format, then it can be done by adding an option to the preferences.
However, if you want to add a possibility to enter time in 24h format (what I originally thought), then it's much more complicated, see: https://github.com/Ulauncher/ulauncher-timer/blob/master/timer/query_parser.py.

I'll be happy to review PRs whether it's the first case or second or both.

@brunetton
Copy link
Contributor Author

If you just want to add an option to display the time in 24h format, then it can be done by adding an option to the preferences.

Indeed :) Happy to see that we agree on that

However, if you want to add a possibility to enter time in 24h format (what I originally thought), then it's much more complicated, see: https://github.com/Ulauncher/ulauncher-timer/blob/master/timer/query_parser.py.

Yes I already read all the code I think and indeed to parse it's a little more complicated. We would really need to use a third party lib as Moment or something similar. But this is not my need for the moment as I only use timers in minutes from now

I'll be happy to review PRs whether it's the first case or second or both.

Great !

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.

Time formatting: why harcoded 12h-format ?
2 participants