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

feat/nice_date utils #42

Merged
merged 4 commits into from
Feb 14, 2023
Merged

feat/nice_date utils #42

merged 4 commits into from
Feb 14, 2023

Conversation

JarbasAl
Copy link
Member

@JarbasAl JarbasAl added the enhancement New feature or request label Nov 27, 2022
@JarbasAl JarbasAl requested a review from NeonDaniel November 27, 2022 15:55
@codecov
Copy link

codecov bot commented Nov 27, 2022

Codecov Report

❗ No coverage uploaded for pull request base (dev@9d6a9bb). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff          @@
##             dev     #42   +/-   ##
=====================================
  Coverage       ?   0.00%           
=====================================
  Files          ?      66           
  Lines          ?   16926           
  Branches       ?       0           
=====================================
  Hits           ?       0           
  Misses         ?   16926           
  Partials       ?       0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@NeonDaniel NeonDaniel left a comment

Choose a reason for hiding this comment

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

Added functions need docstrings. If we're not using a standard date formatting string, valid options should either be tied to an enum class or documented in docstrings; unsupported formats should probably raise an exception

@@ -370,6 +374,39 @@ def nice_date_time(dt, lang='', now=None, use_24hour=False,
use_ampm)


@localized_function(run_own_code_on=[FunctionNotLocalizedError])
def nice_day(dt, date_format='MDY', include_month=True, lang=""):
Copy link
Member

Choose a reason for hiding this comment

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

perhaps date_format should be an enum since there are only 3(?) valid options right now (MDY, DMY, YMD).

Copy link
Member Author

Choose a reason for hiding this comment

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

can't hurt, but note this comes from mycroft.conf and the valid options are basically defined there, so let's define what the enum should contain clearly

return "{} {}".format(month, dt.strftime("%d"))
else:
return "{} {}".format(dt.strftime("%d"), month)
return dt.strftime("%d")
Copy link
Member

Choose a reason for hiding this comment

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

just realized that a one digit day is formatted with "0x". I don't know if all TTS can handle that.
Better dt.strftime("%-d")

else:
raise ValueError("invalid date_format")
return {
"date_string": dtstr,
Copy link
Member

@emphasize emphasize Jan 13, 2023

Choose a reason for hiding this comment

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

I feel we should include abbreviations coming from
nice_month/nice_weekdaywith a changed signature

def nice_month(dt, abbreviation=False, lang=""):

in both cases date_format is irrelevant (as is)

@JarbasAl
Copy link
Member Author

I'm merging this one and addressing feedback later as this is a direct port of utils used in the datetime skill,

I want to merge the homescreen PR needing this to reduce the insane amount of log spam it produces, right now im focused on getting 0.0.7 core release out and dont have time to focus on LF

@JarbasAl JarbasAl merged commit 523f942 into dev Feb 14, 2023
@JarbasAl JarbasAl deleted the feat/nice_dates branch February 14, 2023 17:27
@JarbasAl JarbasAl mentioned this pull request Feb 14, 2023
93 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants