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

[#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

merged 7 commits into from
Oct 28, 2023

Conversation

jq1836
Copy link
Contributor

@jq1836 jq1836 commented Oct 7, 2023 •
edited
Loading

Part of #1936

Proposed commit message

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate repo-sorter.js to TypeScript code to facilitate future
changes to the code.

Other information

Relevant changes made

  • Implemented checkKeyAndGetValue method to retrieve the value stored in element with key, key. If a key is not provided, or if there is no such key in element, it returns undefined. In both these cases, the array is not sorted.

@jq1836 jq1836 marked this pull request as ready for review October 7, 2023 09:13
@chan-j-d chan-j-d requested a review from a team October 8, 2023 17:08
@damithc
Copy link
Collaborator

damithc commented Oct 17, 2023

@reposense/active-reviewers-1 this PR seems to be ready for review

ckcherry23
ckcherry23 approved these changes Oct 21, 2023
View reviewed changes
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

LGTM. Just left a comment about the lint warnings.

frontend/src/utils/repo-sorter.ts Outdated
}
/** 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];
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.

@ckcherry23 ckcherry23 requested a review from a team October 21, 2023 08:19
@jq1836
Copy link
Contributor Author

jq1836 commented Oct 27, 2023

Kindly requesting for your attention in reviewing PRs #2052 and #2053 related to the migration to TypeScript. Once done, issue #1936 can be closed.

@jq1836 jq1836 mentioned this pull request Oct 27, 2023
Merged
@chan-j-d chan-j-d merged commit 4dae85d into reposense:master Oct 28, 2023
10 checks passed
@github-actions
Copy link
Contributor

The following links are for previewing this pull request:

@jq1836 jq1836 deleted the 1936-migrate-repo-sorter branch October 28, 2023 14:39
ckcherry23 pushed a commit that referenced this pull request Jan 8, 2024
@SkyBlaise99 @jq1836
@chan-j-d @dependabot @pratham31012002
[#944] Improve visualization for full and partial credit (#2070)
086a64b 
* [#2027] Fix date range bug (#2034)

Currently, users are unable to select a zoom range that includes 
the until date.

This results in misleading data being presented to users.

* [#2039] Update cypress minimum requirement to 12.15.0 (#2041)

Chrome bug is causing cypress to fail to open a browser on Github 
Actions, causing frontend tests and CI to fail. Upgrading cypress 
to greater than 12.15.0 will fix this issue.

Let's upgrade cypress to fix the failing CI.

* [#1936] Migrate c-segment.vue to typescript (#2035)

Currently, there is still some JavaScript code which remains 
unmigrated. This allows for type unsafe code to be written, 
potentially resulting in unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript 
code to facilitate future changes to the code.

* [#1936] Migrate load-font-awesome-icons.js to typescript (#2040)

Currently, there is still some JavaScript code which remains 
unmigrated. This allows for type unsafe code to be written, 
potentially resulting in unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript 
code to facilitate future changes to the code.

* [#2045] Fix cypress zoom feature test (#2047)

Currently, Cypress zoom feature tests are failing due to a recent change
in behavior caused by a bug fix. With the tests failing, we are unable
to detect any future regressions.

Let's update the Cypress tests to test for the new intended behavior.

* [#1936] Migrate random-color-gen.js to typescript (#2043)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate random-color-generator.js JavaScript code to TypeScript
code to facilitate future changes to the code.

* [#1936] Migrate c-segment-collection.vue to typescript (#2036)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript code to
facilitate future changes to the code.

* [#1936] Migrate c-resizer.vue to typescript (#2038)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript code to
facilitate future changes to the code.

* Bump zod from 3.20.6 to 3.22.3 in /frontend (#2048)

Bumps [zod](https://github.com/colinhacks/zod) from 3.20.6 to 3.22.3.
- [Release notes](https://github.com/colinhacks/zod/releases)
- [Changelog](https://github.com/colinhacks/zod/blob/master/CHANGELOG.md)
- [Commits](colinhacks/zod@v3.20.6...v3.22.3)

---
updated-dependencies:
- dependency-name: zod
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump @cypress/request and cypress in /frontend/cypress (#2042)

Bumps [@cypress/request](https://github.com/cypress-io/request) to 3.0.1 and updates ancestor dependency [cypress](https://github.com/cypress-io/cypress). These dependencies need to be updated together.


Updates `@cypress/request` from 2.88.12 to 3.0.1
- [Release notes](https://github.com/cypress-io/request/releases)
- [Changelog](https://github.com/cypress-io/request/blob/master/CHANGELOG.md)
- [Commits](cypress-io/request@v2.88.12...v3.0.1)

Updates `cypress` from 12.17.4 to 13.3.0
- [Release notes](https://github.com/cypress-io/cypress/releases)
- [Changelog](https://github.com/cypress-io/cypress/blob/develop/CHANGELOG.md)
- [Commits](cypress-io/cypress@v12.17.4...v13.3.0)

---
updated-dependencies:
- dependency-name: "@cypress/request"
  dependency-type: indirect
- dependency-name: cypress
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* [#1936] Migrate c-ramp.vue to typescript (#2037)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript code to
facilitate future changes to the code.

* Give partial credit if annotated author is not the same as the blame
author

* [#2054] Fix zoom view bug (#2055)

Currently, when granularity is set to day or week, clicking on a ramp
will open up a zoom view where commit messages are not being displayed
and sorting by insertions does not result in any sorting. 

Let's fix the unintended behaviour of the zoom view.

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

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate repo-sorter.js to TypeScript code to facilitate future
changes to the code.

* [#1936] Migrate safari_date.js to typescript (#2053)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate safari_date.js to TypeScript code to facilitate future
changes to the code.

* Remove frontend JS lint (#2063)

Currently, frontend linter is failing due to lint scripts 
checking javascript files, the last of which has been 
removed in PR #2053.

Lets update the lint command to exclude javascript 
files front the check.

* use full and partial credit color

* [#1929] Add dynamic positioning support for tooltips (#2056)

Currently, most tooltips are shown above buttons and text. 
When these tooltips appear at the top of the viewport, 
part of the tooltips will not be rendered.

Let's implement changes such that these tooltips appear below the
text or button, when appearing at the top of the viewport.

* Add test cases for annotated author overriding last author's credit

* revert merge from master

* revert merge from master 58b7002

* [#1928] Fix tooltip zIndex such that it doesn't occlude next file title (#2057)

Currently, if one hovers over a tooltip of the pinned title of
a file whose content is scrolled almost completely, such that 
the title of the next file is just below the pinned title, the 
tooltip is not displayed appropriately, as the title of the next 
file obstructs it.

Let's fix this issue.

* [#1726] Update GitHub-specific references in codebase and docs (#2050)

There are still leftover references specific to GitHub on parts of 
the codebase and docs that have been generalized to accept 
other remote git hosts. 

Let's update these GitHub references to use more general language.

* Trigger workflow

* Revert "Merge branch 'master' into 944-analyze-authorship"

This reverts commit 950c912, reversing
changes made to 4bd05a7.

* fix frontend test failing

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: jq1836 <[email protected]>
Co-authored-by: Chan Jun Da <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Pratham Jain <[email protected]>
ckcherry23 pushed a commit that referenced this pull request Jan 8, 2024
@SkyBlaise99 @jq1836
@chan-j-d @dependabot @pratham31012002
[#944] Change to originality score and new threshold value (#2072)
c85423e 
* [#2027] Fix date range bug (#2034)

Currently, users are unable to select a zoom range that includes 
the until date.

This results in misleading data being presented to users.

* [#2039] Update cypress minimum requirement to 12.15.0 (#2041)

Chrome bug is causing cypress to fail to open a browser on Github 
Actions, causing frontend tests and CI to fail. Upgrading cypress 
to greater than 12.15.0 will fix this issue.

Let's upgrade cypress to fix the failing CI.

* [#1936] Migrate c-segment.vue to typescript (#2035)

Currently, there is still some JavaScript code which remains 
unmigrated. This allows for type unsafe code to be written, 
potentially resulting in unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript 
code to facilitate future changes to the code.

* [#1936] Migrate load-font-awesome-icons.js to typescript (#2040)

Currently, there is still some JavaScript code which remains 
unmigrated. This allows for type unsafe code to be written, 
potentially resulting in unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript 
code to facilitate future changes to the code.

* [#2045] Fix cypress zoom feature test (#2047)

Currently, Cypress zoom feature tests are failing due to a recent change
in behavior caused by a bug fix. With the tests failing, we are unable
to detect any future regressions.

Let's update the Cypress tests to test for the new intended behavior.

* [#1936] Migrate random-color-gen.js to typescript (#2043)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate random-color-generator.js JavaScript code to TypeScript
code to facilitate future changes to the code.

* [#1936] Migrate c-segment-collection.vue to typescript (#2036)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript code to
facilitate future changes to the code.

* [#1936] Migrate c-resizer.vue to typescript (#2038)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript code to
facilitate future changes to the code.

* Bump zod from 3.20.6 to 3.22.3 in /frontend (#2048)

Bumps [zod](https://github.com/colinhacks/zod) from 3.20.6 to 3.22.3.
- [Release notes](https://github.com/colinhacks/zod/releases)
- [Changelog](https://github.com/colinhacks/zod/blob/master/CHANGELOG.md)
- [Commits](colinhacks/zod@v3.20.6...v3.22.3)

---
updated-dependencies:
- dependency-name: zod
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump @cypress/request and cypress in /frontend/cypress (#2042)

Bumps [@cypress/request](https://github.com/cypress-io/request) to 3.0.1 and updates ancestor dependency [cypress](https://github.com/cypress-io/cypress). These dependencies need to be updated together.


Updates `@cypress/request` from 2.88.12 to 3.0.1
- [Release notes](https://github.com/cypress-io/request/releases)
- [Changelog](https://github.com/cypress-io/request/blob/master/CHANGELOG.md)
- [Commits](cypress-io/request@v2.88.12...v3.0.1)

Updates `cypress` from 12.17.4 to 13.3.0
- [Release notes](https://github.com/cypress-io/cypress/releases)
- [Changelog](https://github.com/cypress-io/cypress/blob/develop/CHANGELOG.md)
- [Commits](cypress-io/cypress@v12.17.4...v13.3.0)

---
updated-dependencies:
- dependency-name: "@cypress/request"
  dependency-type: indirect
- dependency-name: cypress
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* [#1936] Migrate c-ramp.vue to typescript (#2037)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate the rest of the JavaScript code to TypeScript code to
facilitate future changes to the code.

* Give partial credit if annotated author is not the same as the blame
author

* [#2054] Fix zoom view bug (#2055)

Currently, when granularity is set to day or week, clicking on a ramp
will open up a zoom view where commit messages are not being displayed
and sorting by insertions does not result in any sorting. 

Let's fix the unintended behaviour of the zoom view.

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

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate repo-sorter.js to TypeScript code to facilitate future
changes to the code.

* [#1936] Migrate safari_date.js to typescript (#2053)

Currently, there is still some JavaScript code which remains unmigrated.
This allows for type unsafe code to be written, potentially resulting in
unintended behavior.

Let's migrate safari_date.js to TypeScript code to facilitate future
changes to the code.

* Remove frontend JS lint (#2063)

Currently, frontend linter is failing due to lint scripts 
checking javascript files, the last of which has been 
removed in PR #2053.

Lets update the lint command to exclude javascript 
files front the check.

* use full and partial credit color

* [#1929] Add dynamic positioning support for tooltips (#2056)

Currently, most tooltips are shown above buttons and text. 
When these tooltips appear at the top of the viewport, 
part of the tooltips will not be rendered.

Let's implement changes such that these tooltips appear below the
text or button, when appearing at the top of the viewport.

* Add test cases for annotated author overriding last author's credit

* revert merge from master

* revert merge from master 58b7002

* [#1928] Fix tooltip zIndex such that it doesn't occlude next file title (#2057)

Currently, if one hovers over a tooltip of the pinned title of
a file whose content is scrolled almost completely, such that 
the title of the next file is just below the pinned title, the 
tooltip is not displayed appropriately, as the title of the next 
file obstructs it.

Let's fix this issue.

* [#1726] Update GitHub-specific references in codebase and docs (#2050)

There are still leftover references specific to GitHub on parts of 
the codebase and docs that have been generalized to accept 
other remote git hosts. 

Let's update these GitHub references to use more general language.

* Trigger workflow

* Revert "Merge branch 'master' into 944-analyze-authorship"

This reverts commit 950c912, reversing
changes made to 4bd05a7.

* fix frontend test failing

* switch to originality score and threshold

* update originality threshold

* revert frontend code changes

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: jq1836 <[email protected]>
Co-authored-by: Chan Jun Da <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Pratham Jain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckcherry23 ckcherry23 ckcherry23 approved these changes

Assignees
No one assigned
Labels
s.ToReview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jq1836 @damithc @ckcherry23 @chan-j-d