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

[#2002] Add explicit return type annotations to TypeScript functions in *.vue files #2125

Merged
merged 20 commits into from
Apr 13, 2024

Conversation

supermii2
Copy link
Contributor

@supermii2 supermii2 commented Feb 19, 2024

Part of #2002

Many frontend functions lack return types after migration to
TypeScript, which can cause errors due to TypeScript's attempted type
inference being incorrect. Specifying return type explicitly can allow
TypeScript to catch errors before the code is run.

Let us add return types annotations to TypeScript functions within Vue
files.

@ckcherry23 ckcherry23 requested a review from a team February 19, 2024 06:23
Copy link
Contributor

@jonasongg jonasongg left a comment

Choose a reason for hiding this comment

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

LGTM! We might even want to add a linter rule for explicit return types. Also, I think the title should include the issue number in front!

@jonasongg jonasongg requested a review from a team February 19, 2024 15:20
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.

Great job on working on your first frontend PR! Have left some minor comments to address. I hope adding the types helped you get acquainted well with the frontend codebase!

frontend/src/components/c-ramp.vue Show resolved Hide resolved
frontend/src/components/c-summary-charts.vue Show resolved Hide resolved
@ckcherry23 ckcherry23 requested a review from a team February 20, 2024 18:22
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this! I believe most of the raw .ts files are still lacking return types - it might be too much for one PR and I'm ok with having this PR focus on just the functions in .vue files! In that case, we might want to update the title of this PR to reflect that, and change "Fixes" to "Part of" to prevent this PR from closing the entire issue.

Also as Jonas mentioned, it might be worth looking into if there's an eslint rule that can help enforce the return types! I believe it's possible to only enable the rule for .vue files too, and we can proceed to enable it for general .ts files once that portion has been completed in a separate PR.

frontend/src/app.vue Outdated Show resolved Hide resolved
frontend/src/views/c-authorship.vue Outdated Show resolved Hide resolved
frontend/src/views/c-authorship.vue Outdated Show resolved Hide resolved
@ckcherry23
Copy link
Member

Also, I think the title should include the issue number in front!

@supermii2 Remember to add the issue number in the front of the title!

Copy link
Contributor

The following links are for previewing this pull request:

Commit to be reverted later
@supermii2 supermii2 reopened this Mar 11, 2024
@supermii2 supermii2 changed the title Add explicit return type annotations to TypeScript functions [#2002] Add explicit return type annotations to TypeScript functions Mar 11, 2024
@supermii2
Copy link
Contributor Author

Thanks for your work on this! I believe most of the raw .ts files are still lacking return types - it might be too much for one PR and I'm ok with having this PR focus on just the functions in .vue files! In that case, we might want to update the title of this PR to reflect that, and change "Fixes" to "Part of" to prevent this PR from closing the entire issue.

Also as Jonas mentioned, it might be worth looking into if there's an eslint rule that can help enforce the return types! I believe it's possible to only enable the rule for .vue files too, and we can proceed to enable it for general .ts files once that portion has been completed in a separate PR.

Suggestion added, thank you!

@vvidday
Copy link
Contributor

vvidday commented Mar 11, 2024

Thanks for your changes @supermii2! If we are going ahead with the plan of splitting up the issue into two PRs (one for typescript functions inside Vue components, and one for functions in raw .ts files), can I trouble you to remove the changes to brokenLinkMixin.ts and store.ts in this PR? You can include them in the PR for the raw .ts files.

Additionally, could you update the title of this PR and the action statement (Let's... part of the commit message) to reflect that this PR is only adding return types to functions within Vue components?

The linting rule(s) can be added in a separate PR too, I've made an issue to track it in the meantime: #2156

@supermii2
Copy link
Contributor Author

Also as Jonas mentioned, it might be worth looking into if there's an eslint rule that can help enforce the return types! I believe it's possible to only enable the rule for .vue files too, and we can proceed to enable it for general .ts files once that portion has been completed in a separate PR

Hi, I've had to change the linting rules for unused variables to use the one built for typescript because it was creating false positives. Thank you!

I'll revert the changes done to *.ts in the commit after this comment.

@supermii2 supermii2 changed the title [#2002] Add explicit return type annotations to TypeScript functions [#2002] Add explicit return type annotations to TypeScript functions in *.vue files Mar 13, 2024
@vvidday
Copy link
Contributor

vvidday commented Mar 13, 2024

Also as Jonas mentioned, it might be worth looking into if there's an eslint rule that can help enforce the return types! I believe it's possible to only enable the rule for .vue files too, and we can proceed to enable it for general .ts files once that portion has been completed in a separate PR

Hi, I've had to change the linting rules for unused variables to use the one built for typescript because it was creating false positives. Thank you!

I'll revert the changes done to *.ts in the commit after this comment.

Could you elaborate (maybe with some examples) regarding the false positives, and how changing the rule fixes it? Also, could you check on the grammar of the final sentence in the proposed commit message? Thanks!

frontend/src/main.ts Outdated Show resolved Hide resolved
@MarcusTXK
Copy link
Member

@supermii2 bump on this, I think this PR is almost complete, just need some small changes and we can merge this in

@supermii2
Copy link
Contributor Author

Could you elaborate (maybe with some examples) regarding the false positives, and how changing the rule fixes it? Also, could you check on the grammar of the final sentence in the proposed commit message? Thanks!

    sortingFunction(): (a: Commit, b: Commit) => number {
      const commitSortFunction = this.commitsSortType === CommitsSortType.Time
        ? (commit: Commit): string => commit.date
        : (commit: Commit): number => commit.insertions;

      return (a: Commit, b: Commit): number => (this.toReverseSortedCommits ? -1 : 1)
        * window.comparator(commitSortFunction)(a, b);
    },

Here's a potential false positive found in c-zoom.vue. The default no-unused-vars marks a and b used in the return type as unused variables, which is fixed by using the typescript version.

@vvidday
Copy link
Contributor

vvidday commented Apr 1, 2024

Could you elaborate (maybe with some examples) regarding the false positives, and how changing the rule fixes it? Also, could you check on the grammar of the final sentence in the proposed commit message? Thanks!

    sortingFunction(): (a: Commit, b: Commit) => number {
      const commitSortFunction = this.commitsSortType === CommitsSortType.Time
        ? (commit: Commit): string => commit.date
        : (commit: Commit): number => commit.insertions;

      return (a: Commit, b: Commit): number => (this.toReverseSortedCommits ? -1 : 1)
        * window.comparator(commitSortFunction)(a, b);
    },

Here's a potential false positive found in c-zoom.vue. The default no-unused-vars marks a and b used in the return type as unused variables, which is fixed by using the typescript version.

Got it, thanks for clarifying! There's now merge conflicts introduced by #2132 that have to be resolved before we can merge this PR. Also as mentioned, could you check on the grammar of the final sentence in the proposed commit message? Thanks!

Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

Hi @supermii2, I added the missing return types for the new file (c-zoom-commit-message) & updated the proposed commit message to move this PR along for now.

Please take note for future that whenever you finish the requested changes, do re-request a review and/or let us know the PR is ready for review again.

Thanks again for your work on this PR!

@vvidday vvidday requested a review from ckcherry23 April 4, 2024 07:40
@supermii2
Copy link
Contributor Author

Hi @supermii2, I added the missing return types for the new file (c-zoom-commit-message) & updated the proposed commit message to move this PR along for now.

Please take note for future that whenever you finish the requested changes, do re-request a review and/or let us know the PR is ready for review again.

Thanks again for your work on this PR!

Thank you!

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! Thanks for your work @supermii2. If you'd like to work further on the frontend, you can create a new PR for adding explicit return type annotation for functions in the raw .ts files as well.

@ckcherry23 ckcherry23 merged commit 2692f0f into reposense:master Apr 13, 2024
10 checks passed
Copy link
Contributor

The following links are for previewing this pull request:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants