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

Split audio source error message #216 #238

Merged
merged 6 commits into from
Aug 25, 2023

Conversation

blueuwu
Copy link
Contributor

@blueuwu blueuwu commented Aug 7, 2023

Split the error message into two more descriptive ones that are validated individually.

Copy link
Collaborator

@clabe45 clabe45 left a comment

Choose a reason for hiding this comment

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

Hi, thanks for contributing! Looks great so far

Comment on lines 155 to 159
if (isNaN(this.currentTime)) {
this.source.currentTime = this.sourceStartTime
} else {
this.source.currentTime = this.currentTime + this.sourceStartTime
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please undo this change, it was from another recent PR

@@ -86,11 +86,16 @@ function AudioSourceMixin<OptionsSuperclass extends BaseOptions> (superclass: Co

const load = () => {
// TODO: && ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can go now. Not sure what I was thinking when I wrote that, but it no longer applies.

@clabe45
Copy link
Collaborator

clabe45 commented Aug 7, 2023

Also please run the following command to fix the linter warnings:

npm run fix

@blueuwu
Copy link
Contributor Author

blueuwu commented Aug 8, 2023

Hey Clabe!

I have undone the changes from another PR, removed that comment on line 88, fixed linter warnings and pushed the changes to the PR

Copy link
Collaborator

@clabe45 clabe45 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 addressing those points! One more thing, the second error message should be inverted, because sourceStartTime should be less than or equal to duration

}

if (this.source.duration < this.sourceStartTime) {
throw new Error('Invalid options.sourceStartTime. It must greater than options.source.duration.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be the opposite:

options.sourceStartTime cannot exceed options.source.duration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's alright. If sourceStartTime is greater than source.duration, it will throw an error, which implies it won't throw an error if sourceStartTime is less than or equal to duration

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, sorry for the late reply, starting a new job. Yea sourceStartTime must be less than or equal to duration, but the error message says it must be greater than duration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Congrats on the new job! Alright, I will fix it up immediately

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!!

I meant you should change the error message to be the opposite of what it was. The condition itself was correct. So,

if (this.sourceStartTime > this.source.duration) {
  throw new Error('options.sourceStartTime cannot exceed options.source.duration')
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! Sorry, I have been sick terribly so couldn't attend this issue earlier.

Ah, you meant the Error message itself. I feel dumb now not gonna lie. Let me fix that up real quick, and apologies for the carelessness

Copy link
Collaborator

@clabe45 clabe45 left a comment

Choose a reason for hiding this comment

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

No worries at all haha, thank you!!

@blueuwu blueuwu closed this Aug 25, 2023
@clabe45 clabe45 reopened this Aug 25, 2023
@clabe45
Copy link
Collaborator

clabe45 commented Aug 25, 2023

Hang on, I started running the checks and then something came up. I need to merge this once the checks run again.

@clabe45 clabe45 merged commit 0869db3 into etro-js:master Aug 25, 2023
7 checks passed
mushahidq pushed a commit to mushahidq/etro that referenced this pull request Oct 11, 2024
* added bindings in reference to apid

* updates

* rename toRustCore

* add type alias

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

Successfully merging this pull request may close these issues.

2 participants