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.

Sign up for GitHub

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

Jump to bottom

Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate() #20655

Merged
merged 4 commits into from
Jun 26, 2024
Merged

Type-annotate and unit-test timeformat, fix off-by-one day in parseShortDate() #20655

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c70afc3
pkg: Drop extension in timeformat imports
martinpitt Jun 24, 2024
7f8829b
lib: Type-annotate timeformat
martinpitt Jun 24, 2024
d354edd
base1: Add unit test for timeformat library
martinpitt Jun 24, 2024
d8cf4f4
lib: Fix timeformat.parseShortDate()
martinpitt Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 26 additions & 17 deletions pkg/lib/timeformat.js → pkg/lib/timeformat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,47 +8,56 @@ import { parse, formatDistanceToNow } from 'date-fns';
import * as locales from 'date-fns/locale';

// this needs to be dynamic, as some pages don't initialize cockpit.language right away
export const dateFormatLang = () => cockpit.language.replace('_', '-');
export const dateFormatLang = (): string => cockpit.language.replace('_', '-');
Copy link
Member

Choose a reason for hiding this comment

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

Was it necessary to annotate the return type?

In general, return types can be omitted, and I usually do that. On the other hand, I've seen noises from "smart people" (including the TypeScript team) which suggest that type inference in other parts of the program will be faster if you explicitly annotate...

This isn't a request for change — just random musings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it was necessary -- but if we do this, IMHO we should explicitly type all external API, to make it more self-documenting and avoid ambiguity. I like the python mypy approach to require types in declarations, even if inference could figure them out.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to start writing explicit return types from now on.


const dateFormatLangDateFns = () => {
const dateFormatLangDateFns = (): string => {
if (cockpit.language == "en") return "enUS";
else return cockpit.language.replace('_', '');
};

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const dateFnsLocale = (): any => (locales as any)[dateFormatLangDateFns()];
Copy link
Member

Choose a reason for hiding this comment

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

naja... I guess that part is going away soon...


type Time = Date | number;

// general Intl.DateTimeFormat formatter object
export const formatter = options => new Intl.DateTimeFormat(dateFormatLang(), options);
export const formatter = (options?: Intl.DateTimeFormatOptions) => new Intl.DateTimeFormat(dateFormatLang(), options);
Copy link
Member

Choose a reason for hiding this comment

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

Much better, thanks!


// common formatters; try to use these as much as possible, for UI consistency
// 07:41 AM
export const time = t => formatter({ timeStyle: "short" }).format(t);
export const time = (t: Time): string => formatter({ timeStyle: "short" }).format(t);
// 7:41:26 AM
export const timeSeconds = t => formatter({ timeStyle: "medium" }).format(t);
export const timeSeconds = (t: Time): string => formatter({ timeStyle: "medium" }).format(t);
Copy link
Member

Choose a reason for hiding this comment

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

I demand coverage!!

Copy link
Member

Choose a reason for hiding this comment

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

... or the 🪓

Copy link
Member Author

Choose a reason for hiding this comment

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

pkg/metrics/metrics.jsx:        let tooltip = timeformat.timeSeconds(time) + "\n\n";

we don't test the tooltip, but the qunit test does cover this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

// June 30, 2021
export const date = t => formatter({ dateStyle: "long" }).format(t);
export const date = (t: Time): string => formatter({ dateStyle: "long" }).format(t);
// 06/30/2021
export const dateShort = t => formatter().format(t);
export const dateShort = (t: Time): string => formatter().format(t);
// Jun 30, 2021, 7:41 AM
export const dateTime = t => formatter({ dateStyle: "medium", timeStyle: "short" }).format(t);
export const dateTime = (t: Time): string => formatter({ dateStyle: "medium", timeStyle: "short" }).format(t);
// Jun 30, 2021, 7:41:23 AM
export const dateTimeSeconds = t => formatter({ dateStyle: "medium", timeStyle: "medium" }).format(t);
export const dateTimeSeconds = (t: Time): string => formatter({ dateStyle: "medium", timeStyle: "medium" }).format(t);
// Jun 30, 7:41 AM
export const dateTimeNoYear = t => formatter({ month: 'short', day: '2-digit', hour: '2-digit', minute: '2-digit' }).format(t);
export const dateTimeNoYear = (t: Time): string => formatter({ month: 'short', day: '2-digit', hour: '2-digit', minute: '2-digit' }).format(t);
// Wednesday, June 30, 2021
export const weekdayDate = t => formatter({ dateStyle: "full" }).format(t);
export const weekdayDate = (t: Time): string => formatter({ dateStyle: "full" }).format(t);

// The following options are helpful for placeholders
// yyyy/mm/dd
export const dateShortFormat = () => locales[dateFormatLangDateFns()].formatLong.date({ width: 'short' });
export const dateShortFormat = (): string => dateFnsLocale().formatLong.date({ width: 'short' });
// about 1 hour [ago]
export const distanceToNow = (t, addSuffix) => formatDistanceToNow(t, { locale: locales[dateFormatLangDateFns()], addSuffix });
export const distanceToNow = (t: Time, addSuffix?: boolean): string => formatDistanceToNow(t, {
locale: dateFnsLocale(),
addSuffix: addSuffix ?? false
});

// Parse a string localized date like 30.06.21 to a Date Object
export function parseShortDate(dateStr) {
export function parseShortDate(dateStr: string): Date {
const parsed = parse(dateStr, dateShortFormat(), new Date());

// Strip time which may cause bugs in calendar
const timePortion = parsed.getTime() % (3600 * 1000 * 24);
return new Date(parsed - timePortion);
const time = parsed.getTime();
const timePortion = time % (3600 * 1000 * 24);
return new Date(time - timePortion);
}

/***
Expand All @@ -61,6 +70,6 @@ export function parseShortDate(dateStr) {

const first_dow_sun = ['en', 'ja', 'ko', 'pt', 'pt_BR', 'sv', 'zh_CN', 'zh_TW'];

export function firstDayOfWeek() {
export function firstDayOfWeek(): number {
return first_dow_sun.indexOf(cockpit.language) >= 0 ? 0 : 1;
Copy link
Member Author

Choose a reason for hiding this comment

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

that line is covered, just not both cases -- we dont' test the shutdown time or timer create dialog in non-English. But again, qunit test covers this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

}