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

Prepare for Code Review #124

Closed
samreid opened this issue Sep 4, 2018 · 6 comments
Closed

Prepare for Code Review #124

samreid opened this issue Sep 4, 2018 · 6 comments

Comments

@samreid
Copy link
Member

samreid commented Sep 4, 2018

From #98, it would be nice to get this simulation code reviewed after it is feature complete and before QA dev testing. I'm hoping it will be feature complete before the end of November. Assigned to @ariel-phet to schedule a reviewer.

@ariel-phet
Copy link

@samreid please remember we always do a thorough dev test before code review, so that the reviewer is looking at code that is considered fairly clean and complete.

@ariel-phet ariel-phet assigned samreid and unassigned ariel-phet Sep 4, 2018
@samreid
Copy link
Member Author

samreid commented Sep 5, 2018

That makes sense, thanks for the reminder.

UPDATE: on hold until QA dev test complete.

@samreid samreid changed the title Code Review Preparing for Code Review Oct 8, 2018
@samreid
Copy link
Member Author

samreid commented Oct 8, 2018

Code review should include phetsims/griddle#30 if it is not completed yet.

Also, I renamed this issue to be about preparing for Code Review, in the future we'll create a separate issue with the code review checklist as the top comment.

@samreid samreid changed the title Preparing for Code Review Prepare for Code Review Oct 8, 2018
@samreid
Copy link
Member Author

samreid commented Dec 8, 2018

Draft of instructions for the reviewer. Not ready for review yet, but I'll update this comment as I have more instructions to give.

I recommend to start with the documentation:
wave-interference/doc/implementation-notes.md
wave-interference/doc/model.md

Then the code part of the review should begin with an understanding of these core model files which are central to the sim:
wave-interference/js/waves/model/WavesScreenModel.js
wave-interference/js/common/model/Scene.js
wave-interference/js/common/model/Lattice.js

Please omit wave-interference/js/diffraction from the review. It is a prototype for a screen that will be designed and developed for a future release.

I'm fine with // REVIEW comments or issue comments, but please open a new issue for any problem that is non-trivial or will require discussion or iteration. For any trivial problems that you can fix as you come across them during the review, please fix them and I will review all your commits.

I thought it may be helpful to provide a birds-eye-view of the names of some visible types in the user interface. I'm adding this here as a temporary aid in case it is helpful. Not checking in because it seems a hassle to maintain:

snapshot2

@samreid
Copy link
Member Author

samreid commented Dec 13, 2018

I discussed this with @pixelzoom this afternoon. So far, all of the issues discovered during dev testing have been local/modular in nature, and have not led to architecture changes or reorganization that would disturb a code review. If this pattern holds up, we could start review Monday or Tuesday Dec 17-18. I expect there will still be open issues and/or the dev test may not be complete, but we agreed that starting the code review sooner may be better in light of time constraints the following week.

@samreid samreid mentioned this issue Dec 18, 2018
@samreid
Copy link
Member Author

samreid commented Dec 18, 2018

I opened the code review issue, closing this one.

@samreid samreid closed this as completed Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants