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

Force return types and arrow callbacks on functions #466

Merged
merged 5 commits into from
Jan 2, 2021
Merged

Force return types and arrow callbacks on functions #466

merged 5 commits into from
Jan 2, 2021

Conversation

ferferga
Copy link
Member

@ferferga ferferga commented Dec 30, 2020 •
edited
Loading

Added a rule for enforcing return types on all the functions, which should help us with type checking across all the code

We should also investigate how we can get VueI18n to return strings instead of TranslateResult, so we don't need to invoke toString() or do any string casting.

@ferferga ferferga requested review from heyhippari and camc314 and removed request for heyhippari December 30, 2020 13:22
@codecov-io
Copy link

codecov-io commented Dec 30, 2020 •
edited
Loading

Codecov Report

Merging #466 (dcb69d8) into master (6bb48a0) will not change coverage.
The diff coverage is 0.83%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #466   +/-   ##
======================================
  Coverage    0.62%   0.62%           
======================================
  Files          77      77           
  Lines        2073    2073           
  Branches      294     294           
======================================
  Hits           13      13           
  Misses       2059    2059           
  Partials        1       1
Impacted Files Coverage Δ
components/Buttons/FilterButton.vue 0.00% <0.00%> (ø)
components/Buttons/TypeButton.vue 0.00% <0.00%> (ø)
components/Buttons/UserButton.vue 0.00% <0.00%> (ø)
components/Forms/AddServerForm.vue 0.00% <0.00%> (ø)
components/Forms/LoginForm.vue 0.00% <0.00%> (ø)
components/Item/Card.vue 0.00% <0.00%> (ø)
components/Item/ItemGrid.vue 0.00% <0.00%> (ø)
components/Item/ItemMenu.vue 0.00% <ø> (ø)
components/Item/MediaInfo.vue 0.00% <0.00%> (ø)
components/Item/Metadata/DateInput.vue 0.00% <0.00%> (ø)
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bb48a0...dcb69d8. Read the comment docs.

@ThibaultNocchi
Copy link
Member

Maybe a simple global mixin to wrap the calls to i18n?

@ferferga
Copy link
Member Author

ferferga commented Dec 30, 2020 •
edited
Loading

@ThibaultNocchi Just found this: kazupon/vue-i18n#410 (comment)

Making a commit to replace all occurences

EDIT: I discovered it has two advantages and one disadvange though:

Advantages

  • It only returns strings, so it quit us from managing other boilerplate. This means better typings as mentioned before.
  • Supports pluralization

Disadvantages

  • Value replacement is the third parameter now (instead of the second). This means that we will need to use undefined as a second arg, which is something that we might need to accostume. See here
    I already replaced manually and carefully all the occurences I found, but perhaps I missed something

Overall, anyway, I think it's a good change, as it's also a more feature complete version of $t

@ThibaultNocchi
Copy link
Member

@ferferga In the Vue i18n API, $tc also returns a TranslateResult, so I guess your typings should work with $t

@ferferga
Copy link
Member Author

@ThibaultNocchi That's what they say but doesn't translate into practice 🤷‍♂️

@ferferga ferferga requested a review from heyhippari December 30, 2020 18:46
@ferferga ferferga changed the title Force return types on functions Force return types and arrow callbacks on functions Dec 30, 2020
camc314
camc314 reviewed Jan 1, 2021
View reviewed changes
utils/hlsFormats.ts
function supportsAc3InHls(videoTestElement: HTMLVideoElement) {
function supportsAc3InHls(
videoTestElement: HTMLVideoElement
): boolean | string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
): boolean | string {
): boolean {

Copy link
Member Author

Choose a reason for hiding this comment

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

@camc314 This function also returns the string from the canPlayType https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/canPlayType

utils/mp4AudioFormats.ts
@@ -57,7 +57,7 @@ function hasMp2AudioSupport(): boolean {
* @param {HTMLVideoElement} videoTestElement A HTML video element for testing codecs
* @returns {boolean} Determines if browserr has DTS audio support
*/
function hasDtsSupport(videoTestElement: HTMLVideoElement) {
function hasDtsSupport(videoTestElement: HTMLVideoElement): boolean | string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function hasDtsSupport(videoTestElement: HTMLVideoElement): boolean | string {
function hasDtsSupport(videoTestElement: HTMLVideoElement): boolean {

Copy link
Member Author

Choose a reason for hiding this comment

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

@camc314 Same as above

@ferferga ferferga requested a review from camc314 January 2, 2021 00:26
heyhippari
heyhippari approved these changes Jan 2, 2021
View reviewed changes
Copy link
Contributor

@heyhippari heyhippari left a comment

Choose a reason for hiding this comment

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

LGTM

@ferferga ferferga mentioned this pull request Jan 2, 2021
Merged
@ferferga ferferga force-pushed the force-return-types branch from c409ac9 to dcb69d8 Compare January 2, 2021 14:59
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 2, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@heyhippari heyhippari merged commit e07615c into master Jan 2, 2021
@heyhippari heyhippari deleted the force-return-types branch January 2, 2021 15:06
@ferferga ferferga mentioned this pull request Jan 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heyhippari heyhippari heyhippari approved these changes

@camc314 camc314 Awaiting requested review from camc314

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ferferga @codecov-io @ThibaultNocchi @camc314 @heyhippari