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

Added tests to make sure specs are right side up #14

Merged
merged 2 commits into from
Jan 21, 2020

Conversation

johanndiedrick
Copy link
Contributor

Related to #9

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

fastai2_audio/core.py Outdated Show resolved Hide resolved
@review-notebook-app
Copy link

review-notebook-app bot commented Jan 17, 2020

View / edit / reply to this conversation on ReviewNB (backstory for this conversation format).

rbracco commented on 2020-01-17T21:09:43Z
----------------------------------------------------------------

We don't need a full function here as we are only going to use it as a one-off, so I would rewrite it without the function and documentation to just manually construct a 5_hz tone.


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 17, 2020

View / edit / reply to this conversation on ReviewNB (backstory for this conversation format).

rbracco commented on 2020-01-17T21:09:44Z
----------------------------------------------------------------

This can be removed.


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 17, 2020

View / edit / reply to this conversation on ReviewNB (backstory for this conversation format).

rbracco commented on 2020-01-17T21:09:45Z
----------------------------------------------------------------

can be removed.


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 17, 2020

View / edit / reply to this conversation on ReviewNB (backstory for this conversation format).

rbracco commented on 2020-01-17T21:09:46Z
----------------------------------------------------------------

can be removed


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 17, 2020

View / edit / reply to this conversation on ReviewNB (backstory for this conversation format).

rbracco commented on 2020-01-17T21:09:47Z
----------------------------------------------------------------

can be removed


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 17, 2020

View / edit / reply to this conversation on ReviewNB (backstory for this conversation format).

rbracco commented on 2020-01-17T21:09:48Z
----------------------------------------------------------------

can be removed


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 17, 2020

View / edit / reply to this conversation on ReviewNB (backstory for this conversation format).

rbracco commented on 2020-01-17T21:09:49Z
----------------------------------------------------------------

can be removed


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 17, 2020

View / edit / reply to this conversation on ReviewNB (backstory for this conversation format).

rbracco commented on 2020-01-17T21:09:50Z
----------------------------------------------------------------

remove the 2nd line and move the test into the same cell.


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 17, 2020

View / edit / reply to this conversation on ReviewNB (backstory for this conversation format).

rbracco commented on 2020-01-17T21:09:51Z
----------------------------------------------------------------

This looks good, maybe add a comment saying what it's testing. Also move this and all of the remaining cells down to be with the other tests (right above the "Test warnings for missing/extra arguments") section.


@johanndiedrick
Copy link
Contributor Author

@rbracco I cleaned up the nb with your feedback. Let me know what you think!

@rbracco
Copy link
Collaborator

rbracco commented Jan 21, 2020

Looks great! Merging now. Thank you.

@rbracco rbracco merged commit 16a053d into fastaudio:master Jan 21, 2020
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.

None yet

2 participants