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

Fix various typos #50250

Merged
merged 1 commit into from
Jul 25, 2021
Merged

Fix various typos #50250

merged 1 commit into from
Jul 25, 2021

Conversation

luzpaz
Copy link
Contributor

@luzpaz luzpaz commented Jul 7, 2021

Found via codespell -q 3 -S ./thirdparty,*.po,./DONORS.md -L ackward,ang,ans,ba,beng,cas,childs,childrens,dof,doubleclick,fave,findn,hist,inout,leapyear,lod,nd,numer,ois,ony,paket,seeked,sinc,switchs,te,uint

@luzpaz luzpaz requested review from a team as code owners July 7, 2021 15:31
@KoBeWi
Copy link
Member

KoBeWi commented Jul 7, 2021

If you are fixing comment typos, you could also fix their style (I mean the ones modified). Our preferred style is // Some comment., i.e. double slash, space, capitalized comment and period.

@KoBeWi KoBeWi added this to the 4.0 milestone Jul 7, 2021
@@ -115,7 +115,7 @@ class GodotProcessor extends AudioWorkletProcessor {
this.input = new RingBuffer(p_data[1], avail_in);
this.output = new RingBuffer(p_data[2], avail_out);
} else if (p_cmd === 'stop') {
this.runing = false;
this.running = false;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any other changed occurrence nor any other match, so this might be unused. Poke @Faless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akien-mga see line 129

	process(inputs, outputs, parameters) {
		if (!this.running) {
			return false; // Stop processing.

https://github.com/luzpaz/godot/blob/70bc49e13d90959f7efbfec0a5ddc6a4a8959ce2/platform/javascript/js/libs/audio.worklet.js#L129

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that's actually a bugfix then (or will create a bug, we'll see :D).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please advise how to proceed?

Copy link
Member

Choose a reason for hiding this comment

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

It's fine as is.

Copy link

@jiqz jiqz Jul 7, 2021

Choose a reason for hiding this comment

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

I've checked all mentions of worklets in issues and pull requests in this repository, and don't think there will be any conflicts. The misspelling leads me to believe that a feature was missing, and this feature seems reasonable, so I see no reason not to push unless I have missed something.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good, please squash the commits into one when ready for merging.

@akien-mga
Copy link
Member

If you are fixing comment typos, you could also fix their style (I mean the ones modified). Our preferred style is // Some comment., i.e. double slash, space, capitalized comment and period.

That can be done but it's tedious work while the typos here were fixed by a script. I wouldn't bother until we can convince all core contributors to actually follow that comment style (otherwise we keep having new comments to fix).

@KoBeWi
Copy link
Member

KoBeWi commented Jul 7, 2021

That can be done but it's tedious work while the typos here were fixed by a script.

I mean, there is like 20 modified comments here that use the wrong style. Fixing the style too isn't that much more work.

@akien-mga
Copy link
Member

That can be done but it's tedious work while the typos here were fixed by a script.

I mean, there is like 20 modified comments here that use the wrong style. Fixing the style too isn't that much more work.

My point is that those comments were fixed in 5 s by a script. Changing their style however requires opening the 63 changed files, locating the modified lines and changing the style manually -> 10 min of work.

It's OK if OP is willing to do it but it's not a requirement for me in this PR.

@luzpaz
Copy link
Contributor Author

luzpaz commented Jul 7, 2021

Added requested revisions in e185824

@akien-mga
Copy link
Member

Please squash the commits into one, see PR workflow for instructions.

Copy link

@jiqz jiqz left a comment

Choose a reason for hiding this comment

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

Various comment style fixes.

@akien-mga
Copy link
Member

Force pushed a rebase to squash the 28 commits and solve merge conflicts.

@luzpaz
Copy link
Contributor Author

luzpaz commented Jul 25, 2021

@akien-mga please advise

@akien-mga
Copy link
Member

I'll fix it up again, you repushed all old commits on top of my earlier rebase so it can't be merged as is.

Found via `codespell -q 3 -S ./thirdparty,*.po,./DONORS.md -L ackward,ang,ans,ba,beng,cas,childs,childrens,dof,doubleclick,fave,findn,hist,inout,leapyear,lod,nd,numer,ois,ony,paket,seeked,sinc,switchs,te,uint`
@luzpaz
Copy link
Contributor Author

luzpaz commented Jul 25, 2021

💩
Sorry about that!

@akien-mga akien-mga merged commit 2f221e5 into godotengine:master Jul 25, 2021
@akien-mga
Copy link
Member

Thanks!

@luzpaz luzpaz deleted the typos branch July 25, 2021 11:09
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.

4 participants