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

Documentation pass on files in Mixer folder #923

Merged
merged 11 commits into from
Jul 24, 2021
Merged

Documentation pass on files in Mixer folder #923

merged 11 commits into from
Jul 24, 2021

Conversation

cugone
Copy link
Contributor

@cugone cugone commented Jul 15, 2021

Ref: #861

Smaller chunk of #899.

I plan on splitting the documentation fixes (re: deletions...) into smaller chunks by folder to avoid large-scale merge conflicts.

DanRStevens
DanRStevens reviewed Jul 15, 2021
View reviewed changes
Copy link
Collaborator

@DanRStevens DanRStevens left a comment

Choose a reason for hiding this comment

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

I had a few technical comments that might be wide reaching in their impact.

NAS2D/Mixer/Mixer.h Outdated Show resolved Hide resolved
NAS2D/Mixer/Mixer.h Outdated Show resolved Hide resolved
DanRStevens
DanRStevens reviewed Jul 15, 2021
View reviewed changes
NAS2D/Mixer/Mixer.h Outdated Show resolved Hide resolved
NAS2D/Mixer/Mixer.h Outdated Show resolved Hide resolved
NAS2D/Mixer/Mixer.h Outdated Show resolved Hide resolved
NAS2D/Mixer/Mixer.h Show resolved Hide resolved
@cugone cugone requested a review from DanRStevens July 20, 2021 18:33
DanRStevens
DanRStevens reviewed Jul 21, 2021
View reviewed changes
NAS2D/Mixer/Mixer.h Outdated Show resolved Hide resolved
@cugone cugone requested a review from DanRStevens July 21, 2021 21:57
@cugone
Copy link
Contributor Author

cugone commented Jul 24, 2021

Poke

The status of this affects work starting on #943, #944 and #945. Additionally I don't want to start the next folder until the previous is merged due to conflicts and adding too much to reviewers' plates.

DanRStevens
DanRStevens reviewed Jul 24, 2021
View reviewed changes
Copy link
Collaborator

@DanRStevens DanRStevens left a comment

Choose a reason for hiding this comment

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

I've only got one small concern left, and it looks like it can be sidestepped.

NAS2D/Mixer/Mixer.h Outdated Show resolved Hide resolved
DanRStevens
DanRStevens approved these changes Jul 24, 2021
View reviewed changes
Copy link
Collaborator

@DanRStevens DanRStevens left a comment

Choose a reason for hiding this comment

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

Ok, I agree with all these changes.

And wow, looking back, it's amazing how much discussion was generated from mostly just deleting some comments that don't really say much.

@DanRStevens DanRStevens merged commit 4ac1307 into lairworks:master Jul 24, 2021
@DanRStevens
Copy link
Collaborator

Ehh, build failure on master due to the : added to the documentation strings:
https://app.circleci.com/pipelines/github/lairworks/nas2d-core/1760/workflows/86d177a3-59d4-432a-ae86-ce3b9b7ca530/jobs/7157

Clang does actually check for certain Doxygen errors when -Wdocumentation is passed.

@DanRStevens DanRStevens mentioned this pull request Jul 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanRStevens DanRStevens DanRStevens approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@cugone @DanRStevens