Skip to content

i2s_dma update #248

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

Closed
wants to merge 1 commit into from
Closed

Conversation

linrjing
Copy link

@SidLeung @calvinatintel a word mistake need to be updated.

@bigdinotech
Copy link
Contributor

Thanks @linrjing.
@calvinatintel is the summary good enough for you?
If so it looks good to me

@calvinatintel
Copy link
Contributor

@eriknyquist is our stylist

@eriknyquist
Copy link
Contributor

Hi @linrjing, please refer to commit message guidelines here. Specifically, If you could rewrite the message to describe your changes, e.g. "CurieI2SDMA.h: Fix typo", we would appreciate it!

@kitsunami kitsunami added this to the Castor milestone Jul 25, 2016
@linrjing linrjing force-pushed the feature/i2s_dma_update1 branch from dce20d8 to 41736d2 Compare July 26, 2016 02:07
@linrjing
Copy link
Author

Hi @eriknyquist, the commit message has been amended.

@linrjing linrjing force-pushed the feature/i2s_dma_update1 branch from 41736d2 to b4e7589 Compare July 26, 2016 09:47
@eriknyquist
Copy link
Contributor

@linrjing thank you. @calvinatintel +1

@eriknyquist
Copy link
Contributor

eriknyquist commented Jul 26, 2016

@linrjing actually not quite. While ammending the commit you also added a new change that was not there originally (the comment at line 76 in I2SDMA_RXCallBack.ino).

Additionally, this comment is quite confusing; it does not really help in the understanding of the code that follows, and it will only confuse inexperienced users (I'm experienced and it confused me...).

If you feel the need to explain this block of code with a comment, then I agree that this is a good thing to do, but just remember you are writing for inexperienced users. Pretend you are explaining to a child, not an experienced developer.

Also, please separate this unrelated change into another commit, or at least mention it in your commit message.

CurieI2SDMA.h: Fix typo at line 23 and 24; I2SDMA_RXCallBack.ino: add
explanatory notes to explain how to check the verification of data
@linrjing linrjing force-pushed the feature/i2s_dma_update1 branch from b4e7589 to 3fa4c88 Compare July 27, 2016 02:41
@linrjing
Copy link
Author

linrjing commented Jul 27, 2016

Hi @eriknyquist ,thanks for your help. The commit has been amended. More detail explanatory notes for the I2SDMA_RXCallBack.ino codes have been added.

@eriknyquist
Copy link
Contributor

@linrjing thank you, it looks OK now. @calvinatintel go ahead and merge.

@calvinatintel
Copy link
Contributor

cherry-picked

@eriknyquist
Copy link
Contributor

Github prettified the commit message and fooled me into thinking your message was formatted correctly. It's not. That's my fault for not spotting it. @linrjing please refer to our commit message guidelines in future, I posted the link in an earlier comment but here it is again just in case: https://github.com/01org/corelibs-arduino101/wiki/Writing-a-commit-message

Specifically, you must always leave the 2nd line blank. Type git log --oneline inside a local version that contains this commit, and you will see why this convention exists.

@linrjing
Copy link
Author

linrjing commented Jul 28, 2016

Sorry, @eriknyquist. After I type "git log --oneline", I found the "description: " was at the same line with "subject: ". When I amended the commit, I should have add a blank line after "subject: " line. Currently what can I do? The code has been merged.

@eriknyquist
Copy link
Contributor

@linrjing it's fine, like you said it's already merged. Can't do anything without re-writing the public history, which is a bad idea. No worries, just remember the guidelines next time please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants