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

Add CONTRIBUTING Guidelines #627

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

UnknownSuperficialNight
Copy link
Contributor

@UnknownSuperficialNight UnknownSuperficialNight commented Oct 4, 2024

This pull request introduces a CONTRIBUTING guidelines document to assist potential contributors in understanding how to effectively participate in Rodio. By providing guidelines, the is aim to encourage collaboration and ensure that contributions are of higher quality, while also reducing the effort required to understand the codebase.

Next Steps:
Please review the changes and provide any feedback or suggestions for improvement.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
- Start with tests for new features
- Open draft PR after initial functionality
- Refactor, benchmark, and optimize
- Offer support for test creation
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

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

havent seen the latest changes yet, getting on that now. Overal it looks very nice!

- Encourage use of temporary unit tests during development
- Clarify that rough, non-comprehensive tests are acceptable
- Allow inclusion of temporary tests in pull requests
- Explain rationale for removing tests before final merge:
  - Easier refactoring
  - Reduced necessity due to Rust's type system
- Maintain instruction to run tests with 'cargo test'
@UnknownSuperficialNight
Copy link
Contributor Author

havent seen the latest changes yet, getting on that now. Overal it looks very nice!

Thanks 😌

- Write unit tests for each new function (if applicable)
- Add integration tests for end-to-end scenarios
- Feel free to write temporary unit tests during development if they help you verify functionality
- These tests can be rough and don't need to be comprehensive - they're just development aids
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really like the wording here, it feels welcoming and inclusive!

Copy link
Collaborator

Choose a reason for hiding this comment

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

we just need some instruction's/help for integration tests, those can be hard to write. This one

fn seek_does_not_break_channel_order(
took me a ton of time. We should not expect contributors to write that involved integration tests but it would be very nice if the could.

The wav test is a good example to point to I think: https://github.com/RustAudio/rodio/blob/master/tests/wav_test.rs

Copy link
Collaborator

Choose a reason for hiding this comment

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

took care of it, please take a look let me know what you think

@dvdsk
Copy link
Collaborator

dvdsk commented Oct 4, 2024

off topic, I made a draft for a announcement for the next version of rodio in which we ask for help in improving rodio's API. Let me know if you have any feedback on it: https://github.com/RustAudio/rodio/blob/master/outreach/v0.20_announcement_and_call_for_userstories.md (I should have made it a pr, silly me). You can make a new issue, post the feedback here or even make a pr with changes. Whatever is easiest for you.

CONTRIBUTING.md Outdated
1. Create a new file in `src/source/`
2. Implement the `Source` trait to define how audio samples are generated or modified
3. Consider implementing sources like oscillators, noise generators, or effects like amplification, filtering, or distortion
4. Begin with a test for your new feature. This approach ensures your PR is ready and simplifies development. Don't worry about optimizing initially; focus on functionality.
Copy link
Collaborator

Choose a reason for hiding this comment

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

somewhere in between here we should add:

sources that creates sound get a public (factory) function that constructs the source. This is where you document it. Source that apply effects to other sources get a method with default implementation in the Source trait, see: src/source/mod.rs. This method should be the only way to apply the effect and is where it is documented.

This is as short as I could get it

@dvdsk
Copy link
Collaborator

dvdsk commented Oct 5, 2024

I added some stuff, github actions bot stole the credit stuff. So now I know what the "maintainers can edit this pr" checkbox does, is makes github steal my credit 😢.

edit: kinda fixed it.

… section

Specifically notes some example tests and how they test. Also mentions
sometimes you just have to listen, recommends adding example for that.

Author:    dvdsk <[email protected]>
Date:      Sat Oct 5 02:06:39 2024 +0200
@dvdsk dvdsk force-pushed the add-contributing-guidelines branch from 934d14e to dd3e8de Compare October 5, 2024 00:43
@dvdsk
Copy link
Collaborator

dvdsk commented Oct 5, 2024

we should edit the outreach/announcement <...>.md once this is merged. This might pull some potential contributors over the line.

- Link to CONTRIBUTING.md in project root
- New Integration Tests section:
  - Avoid sound output in tests
  - Tips for testing audio sources
  - Suggest examples for audible features
  - Note challenges in automated audio testing
- Incorporate Integration Tests contributed by dvdsk
- Add leading numbers to bullet points mistakenly commited without numbering
@UnknownSuperficialNight
Copy link
Contributor Author

I added some stuff, github actions bot stole the credit stuff. So now I know what the "maintainers can edit this pr" checkbox does, is makes github steal my credit 😢.

edit: kinda fixed it.

Oof I already made my own version did not realise your version existed so I incorporated yours into mine let me know what u think

@dvdsk
Copy link
Collaborator

dvdsk commented Oct 5, 2024

Oof I already made my own version did not realise your version existed

😆

so I incorporated yours into mine let me know what u think

its better then what I had, only feedback I can think of is changing the current ## Testing to ### Unit Tests But its okay so too, you can remove my bit.

And then I think we are done right?

@UnknownSuperficialNight UnknownSuperficialNight marked this pull request as ready for review October 6, 2024 02:40
@UnknownSuperficialNight
Copy link
Contributor Author

Looks about done, if we can think of anything in the future, we can just make a PR and add it.

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