-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Proof POWERED_FLIGHT_SUBROUTINES #624 #754
Proof POWERED_FLIGHT_SUBROUTINES #624 #754
Conversation
This was recently updated due to the wording being incorrect (well unhelpful and somewhat contradictory) - it's the general usage of spaces, but whatever the scans uses is what you use. |
Looks good to me. Thanks! |
Good morning @wopian & @chrislgarry! If @wopian 's comment is the way it should be, then my contributions are wrong, but easily fixed and it's only white space. If you like I can fix it and open another pull request. Though if that is the case I think that should be updated in the CONTRIBUTING.md file so it is clear that we want white space to match also. I'm happy to do this if it is desired, especially since I'm going to be doing more of these in the next week or so and I want them to be right. p.s. it's really fun reading these comments, I'm not super much for editing or proofreading in general, it's nice to read code from exceptional engineers! |
@wopian has the final word. I may have missed the whitespace nuances since I was reviewing on my phone during my commute home. Regardless, we would be happy if you could submit a new PR accordingly. |
@serialhex I've reverted this PR. Unfortunately GitHub permanently closes PRs once they're merged, so you'll need to create a new branch off of this branch to open a new PR.
Edit: PR was already opened 😅 |
Proofread all comments and code. There are a few comments relating to white space.
I call this out specifically because the scans (at least for this file) seem to use 4 spaces for indentation and 3 spaces for new sentences. For example, in the following image the leading
C
inCDUTRIG
would be over theI
inCOSINES
if it used 3 spaces instead of 4, but instead it is over theE
. Similarly, on the 4th line (there's and example on the 3rd but it's easier to see on the 4th) the period at the end of the line corresponds to a space above it, and then there are 3 spaces before theT
inTHESE
starts.Other than that this PR should be bug-compatible with the scans! 😜