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

[#1936] Migrate repo-sorter.js to typescript #2052

Merged
merged 7 commits into from
Oct 28, 2023
Merged

[#1936] Migrate repo-sorter.js to typescript #2052

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b16439b
Migrate repo-sorter.js to typescript
jq1836 Oct 7, 2023
f5adc38
Remove any type in checkKeyAndGetValue
jq1836 Oct 7, 2023
ca7fd78
Merge branch 'master' into 1936-migrate-repo-sorter
jq1836 Oct 23, 2023
b10e08f
Apply generics to remove any cast
jq1836 Oct 23, 2023
93c4856
Merge branch '1936-migrate-repo-sorter' of https://github.com/jq1836/…
jq1836 Oct 23, 2023
d913ad1
Remove cypress downloads file
jq1836 Oct 23, 2023
ce8ba5e
Merge branch 'master' into 1936-migrate-repo-sorter
chan-j-d Oct 28, 2023
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
78 changes: 60 additions & 18 deletions frontend/src/utils/repo-sorter.js → frontend/src/utils/repo-sorter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,27 @@
function getTotalCommits(total, group) {
return total + group.checkedFileTypeContribution;
import { User } from '../types/types';
import { FilterGroupSelection } from '../types/summary';

function getTotalCommits(total: number, group: User) {
// If group.checkedFileTypeContribution === undefined, then we treat it as 0 contribution
return total + (group.checkedFileTypeContribution ?? 0);
}

function getGroupCommitsVariance(total, group) {
function getGroupCommitsVariance(total: number, group: User) {
return total + group.variance;
}

function sortingHelper(element, sortingOption) {
function checkKeyAndGetValue(element: object, key: string | undefined) {
if (key === undefined || !(key in element)) {
return undefined;
}
/** Casting here is safe as guard clause above ensures that element has an attribute with the same name as the string
* represented by key. */
return (element as any)[key];

Check warning on line 19 in frontend/src/utils/repo-sorter.ts

View workflow job for this annotation

GitHub Actions / Cypress frontend tests 

Unexpected any. Specify a different type

Check warning on line 19 in frontend/src/utils/repo-sorter.ts

View workflow job for this annotation

GitHub Actions / ubuntu-20.04 JDK 8 

Unexpected any. Specify a different type

Check warning on line 19 in frontend/src/utils/repo-sorter.ts

View workflow job for this annotation

GitHub Actions / ubuntu-20.04 JDK 8 

Unexpected any. Specify a different type

Check warning on line 19 in frontend/src/utils/repo-sorter.ts

View workflow job for this annotation

GitHub Actions / ubuntu-22.04 JDK 8 

Unexpected any. Specify a different type

Check warning on line 19 in frontend/src/utils/repo-sorter.ts

View workflow job for this annotation

GitHub Actions / macos-11 JDK 8 

Unexpected any. Specify a different type

Check warning on line 19 in frontend/src/utils/repo-sorter.ts

View workflow job for this annotation

GitHub Actions / macos-12 JDK 8 

Unexpected any. Specify a different type

Check warning on line 19 in frontend/src/utils/repo-sorter.ts

View workflow job for this annotation

GitHub Actions / windows-2019 JDK 8 

Unexpected any. Specify a different type

Check warning on line 19 in frontend/src/utils/repo-sorter.ts

View workflow job for this annotation

GitHub Actions / windows-2022 JDK 8 

Unexpected any. Specify a different type
Copy link
Member

Choose a reason for hiding this comment

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

See if it is possible to use generics here.
Else disable eslint for this line using // eslint-disable-next-line @typescript-eslint/no-explicit-any.

Copy link
Contributor Author

@jq1836 jq1836 Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review @ckcherry23, I have experimented with making use of generics over here and it seems like there is no real need for generics (although I have left a function that uses generics for general use). All that is needed would be casting the key string into a keyof User. However, this causes the problem of handling invalid keys and values that are not ComparablePrimitive, i.e. string | number to resurface (it was being suppressed due to the any typecast previously used). I have tried to handle these cases, but am unsure whether such behavior is appropriate (details below). Do let me know if the current behavior is not as intended.

Details of behavior

  • When a key that is not valid or simply if a key is not provided to the function, checkKeyAndGetValue will return undefined, which will be caught in the getComparablePrimitive function's guard clause, resulting in an empty string being returned.
  • When a key that is associated with a value that is not a ComparablePrimitive, i.e. not a string | number, is provided to the function, it is caught in the getComparablePrimitive function's guard clause, also resulting in an empty string being returned.
  • When an empty string is returned, this results in the associated User being sorted to the front. If all Users in the User[] result in an empty string, no sorting occurs.

}

/** Array is not sorted when sortingOption is not provided. sortingOption is optional to allow it to fit the
* SortingFunction<T> interface. */
function sortingHelper(element: User[], sortingOption?: string): string | number {
if (sortingOption === 'totalCommits') {
return element.reduce(getTotalCommits, 0);
}
Expand All @@ -16,21 +31,28 @@
if (sortingOption === 'displayName') {
return window.getAuthorDisplayName(element);
}
return element[0][sortingOption];
return checkKeyAndGetValue(element[0], sortingOption);
}

function groupByRepos(repos, sortingControl) {
const sortedRepos = [];
function groupByRepos(
repos: User[][],
sortingControl: {
sortingOption: string;
sortingWithinOption: string;
isSortingDsc: string;
isSortingWithinDsc: string; },
) {
const sortedRepos: User[][] = [];
const {
sortingWithinOption, sortingOption, isSortingDsc, isSortingWithinDsc,
} = sortingControl;
const sortWithinOption = sortingWithinOption === 'title' ? 'displayName' : sortingWithinOption;
const sortOption = sortingOption === 'groupTitle' ? 'searchPath' : sortingOption;
repos.forEach((users) => {
if (sortWithinOption === 'totalCommits') {
users.sort(window.comparator((ele) => ele.checkedFileTypeContribution));
users.sort(window.comparator((ele) => ele.checkedFileTypeContribution ?? 0));
} else {
users.sort(window.comparator((ele) => ele[sortWithinOption]));
users.sort(window.comparator((ele) => checkKeyAndGetValue(ele, sortWithinOption)));
}

if (isSortingWithinDsc) {
Expand All @@ -45,8 +67,13 @@
return sortedRepos;
}

function groupByNone(repos, sortingControl) {
const sortedRepos = [];
function groupByNone(
repos: User[][],
sortingControl: {
sortingOption: string;
isSortingDsc: string; },
) {
const sortedRepos: User[] = [];
const { sortingOption, isSortingDsc } = sortingControl;
const isSortingGroupTitle = sortingOption === 'groupTitle';
repos.forEach((users) => {
Expand All @@ -61,7 +88,7 @@
if (sortingOption === 'totalCommits') {
return repo.checkedFileTypeContribution;
}
return repo[sortingOption];
return checkKeyAndGetValue(repo, sortingOption);
}));
if (isSortingDsc) {
sortedRepos.reverse();
Expand All @@ -70,9 +97,16 @@
return sortedRepos;
}

function groupByAuthors(repos, sortingControl) {
const authorMap = {};
const filtered = [];
function groupByAuthors(
repos: User[][],
sortingControl: {
sortingOption: string;
sortingWithinOption: string;
isSortingDsc: string;
isSortingWithinDsc: string; },
) {
const authorMap: { [userName: string]: User[] } = {};
const filtered: User[][] = [];
const {
sortingWithinOption, sortingOption, isSortingDsc, isSortingWithinDsc,
} = sortingControl;
Expand All @@ -89,9 +123,9 @@
});
Object.keys(authorMap).forEach((author) => {
if (sortWithinOption === 'totalCommits') {
authorMap[author].sort(window.comparator((repo) => repo.checkedFileTypeContribution));
authorMap[author].sort(window.comparator((repo) => repo.checkedFileTypeContribution ?? 0));
} else {
authorMap[author].sort(window.comparator((repo) => repo[sortWithinOption]));
authorMap[author].sort(window.comparator((repo) => checkKeyAndGetValue(repo, sortingWithinOption)));
}
if (isSortingWithinDsc) {
authorMap[author].reverse();
Expand All @@ -106,7 +140,15 @@
return filtered;
}

function sortFiltered(filtered, filterControl) {
function sortFiltered(
filtered: User[][],
filterControl: {
filterGroupSelection: FilterGroupSelection;
sortingOption: string;
sortingWithinOption: string;
isSortingDsc: string;
isSortingWithinDsc: string; },
) {
const { filterGroupSelection } = filterControl;
let full = [];

Expand Down
Loading