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

Ensure that all TODOs have an owner. #133

Merged
merged 1 commit into from
Sep 16, 2016
Merged

Ensure that all TODOs have an owner. #133

merged 1 commit into from
Sep 16, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Sep 12, 2016

In the future, all TODOs added either need a bug number (TODO(#NN)) or a
person's github user name. By default, I made irungentoo the owner of
all toxcore TODOs, mannol the owner of toxav TODOs, and myself the owner
of API TODOs.


This change is Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 12, 2016

:lgtm_strong:


Reviewed 26 of 26 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 13, 2016

:lgtm_strong:


Reviewed 19 of 19 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@GrayHatter
Copy link

Reviewed 6 of 16 files at r4.
Review status: 22 of 32 files reviewed at latest revision, 1 unresolved discussion.


toxcore/Messenger.c, line 2442 at r4 (raw file):

/* Minimum messenger run interval in ms
   TODO(irungentoo): A/V */

this should either be removed, or assigned to @mannol

The argument for removing: toxav doesn't need additional or less time, even if we do need to reduce this number for toxav (unlikely IMO) then it should be renamed to messenger min and we should ignore it when toxav is active.

finally; this should be dropped completely. Messenger isn't smarter than crypto_run_interval. and if it is, we should fix that, no this.


Comments from Reviewable

@GrayHatter
Copy link

:lgtm_strong:


Reviewed 10 of 16 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Sep 16, 2016

Review status: 31 of 32 files reviewed at latest revision, 1 unresolved discussion.


toxcore/Messenger.c, line 2442 at r4 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

this should either be removed, or assigned to @mannol

The argument for removing: toxav doesn't need additional or less time, even if we do need to reduce this number for toxav (unlikely IMO) then it should be renamed to messenger min and we should ignore it when toxav is active.

finally; this should be dropped completely. Messenger isn't smarter than crypto_run_interval. and if it is, we should fix that, no this.

Assigned to mannol. I'm not changing code behaviour in this PR.

Comments from Reviewable

In the future, all TODOs added either need a bug number (TODO(#NN)) or a
person's github user name. By default, I made irungentoo the owner of
all toxcore TODOs, mannol the owner of toxav TODOs, and myself the owner
of API TODOs.
@iphydf iphydf closed this Sep 16, 2016
@iphydf iphydf deleted the todo branch September 16, 2016 12:01
@iphydf iphydf merged commit 1494e47 into TokTok:master Sep 16, 2016
@iphydf iphydf modified the milestone: v0.0.1 Nov 5, 2016
@nurupo nurupo mentioned this pull request Nov 15, 2016
This pull request was closed.
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.

3 participants