Skip to content

I added doc strings to 7 files #67

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

Merged
merged 8 commits into from
Mar 6, 2023
Merged

I added doc strings to 7 files #67

merged 8 commits into from
Mar 6, 2023

Conversation

markdicks
Copy link
Contributor

There were 3 files that I added doc strings for improved code readability and maintainability.

@markdicks
Copy link
Contributor Author

I would love to work on more files and add more doc strings if you keen, just comment and let me know

@cloudwebrtc
Copy link
Member

Yes, I'd love to accept PRs for these awesome docs, please go ahead.

@markdicks
Copy link
Contributor Author

Awesome thank you

@markdicks markdicks closed this Mar 6, 2023
@markdicks markdicks reopened this Mar 6, 2023
@markdicks
Copy link
Contributor Author

My bad I closed it thinking it was merged, apologies

@cloudwebrtc
Copy link
Member

cloudwebrtc commented Mar 6, 2023

I can merge now, but if you want to continue adding other files, I will review them later

@markdicks
Copy link
Contributor Author

In about 10-12 hours I will be back to continue with the rest of the files in the libwebrtc/src folder. Hopefully finish those, for now you can merge if you comfortable with that.

@markdicks
Copy link
Contributor Author

Then do lemme know which files you'd like me to focus on then I will do those as well.

@cloudwebrtc
Copy link
Member

In terms of priority, the API documentation in libwebrtc/include is the most needed to add docs, you can start here If you want, most APIs can refer to https://developer.mozilla.org/en-US/docs/Web /API/WebRTC_API is the calling method,

@markdicks
Copy link
Contributor Author

The two deletions are coming from the 'include/rtc_audio_device.h' file, there were two comments that I removed, about 4 words each. I removed them because it seemed covered in the new doc strings I added in both deletions. Hope you don't mind that minor change. Your code still remains unaffected.

On that I have done 4 files in the folder you suggested and went in depth for all four. I hope to continue later on today, you can merge as this is the first load of doc strings. I hope to do 4 big batches like this. Hope you are satisfied. Talk soon xD

@cloudwebrtc
Copy link
Member

lgtm!

@cloudwebrtc cloudwebrtc merged commit 5a430ab into webrtc-sdk:master Mar 6, 2023
@cloudwebrtc
Copy link
Member

@markdicks Can you create a PR in branch if possible? This saves you from triggering an update to the PR when rolling the master branch forward.
For example change
image
to docs/api-docs-for-xxxx

@markdicks markdicks changed the title I added doc strings to 3 files I added doc strings to 7 files Mar 6, 2023
@markdicks
Copy link
Contributor Author

Okay will do from now on, thank you

@markdicks
Copy link
Contributor Author

Good day, hope you doing well today. Sorry for my lack of knowledge, I am not sure what is meant when you say create a PR in the branch. Please do let me know, and then furthermore I did not make any changes to the master branch on my side after you merged but it still tells me that I am 9 commits ahead. Sorry about this, I tried asking a work colleague about it but even they was not sure what to do.

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