This repository has been archived by the owner on Apr 12, 2024. It is now read-only.
Update code review documentation with additional information #9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change set adds several additions to our code review documentation that expand on the following concepts: - Benefits of code reviews - Potential obstacles of instituting code reviews - What to look for when reviewing code There are several document formatting updates made as well.
| review to pass will take time and commitment from all team members if they | ||
| have not already bought into the benefits of TDD. | ||
| - Additional training may be needed for folks who are unfamiliar with | ||
| what linters and/or testing harnesses. |
There was a problem hiding this comment.
this is an incomplete sentence -- maybe zap the word what ?
There was a problem hiding this comment.
Yup, extraneous word left in there, thanks!
|
Thanks for allowing us to collectively hash much of this out in your PR, @ccostino! |
|
@cmc333333: Absolutely, there's a lot of great discussion here and I am looking forward to diving into these responses further when I have a moment again! |
|
Lots of great discussion and feedback here, I am going to incorporate it all in another commit later tonight for further review, thank you everyone! |
This change set builds on the previous changes submitted for our code review documentation based on the feedback and corrections given: - Removed the obstacles section and incorporated concepts into existing sections - Removed redundant statements - Removed items under debate (running code manually and detailed commit messages) - Fixed several grammatical errors - Removed old link to checklist bookmarklet
|
This looks awesome! Hopefully from here on out we can keep PRs to this repo smaller so people can debate issues one at a time. Thank you, Carlo, for dealing with a zillion different threads on this PR, I think that after it is merged we will have a good base to work off of !! 🚢 |
|
Thanks, @jessieay! Glad to have fielded a bunch of things and work everything in that made sense, and there's plenty of material to move forward with on future improvements/guidelines. :-) |
|
Consensus met, merge is go! |
cmc333333
added a commit
that referenced
this pull request
Nov 30, 2015
Update code review documentation with additional information
ryanhofdotgov
added a commit
that referenced
this pull request
Sep 11, 2020
Add anchor data for other TTS orgs
ryanhofdotgov
added a commit
that referenced
this pull request
Sep 11, 2020
Squashed commit of the following: commit 4d63ae3 Merge: 45ec357 6b0f565 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 14:33:15 2020 -0700 Merge pull request #29 from 18F/rmh-gitattributes-glob Fix glob used in .gitattributes commit 6b0f565 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 14:29:34 2020 -0700 Fix glob used in .gitattributes .gitattributes patterns use same convention as .gitignore, where trailing `/**` is used to match everything inside a directory. commit 45ec357 Merge: cf3e0a1 849e5c3 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 10:25:42 2020 -0700 Merge pull request #27 from 18F/rmh-add-ci-cache Add dependency caching in CircleCI commit 849e5c3 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 10:15:56 2020 -0700 Add dependency caching in CircleCI commit cf3e0a1 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 09:18:09 2020 -0700 Remove remaining next steps and migrate to issues commit bb58c7f Merge: c06c47d 2c6f1c0 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 09:11:12 2020 -0700 Merge pull request #21 from 18F/rmh-ci-tests Add tests that get run in CircleCI commit 2c6f1c0 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 08:54:40 2020 -0700 Added missing commas commit 0c691f1 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 08:52:23 2020 -0700 Add ignore URLs from application.js commit 871dd08 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 08:08:02 2020 -0700 Validate HTML, check internal links in CircleCI Adopting approach used in 18F/before-you-ship commit c06c47d Merge: 8b0257d db465de Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 08:03:07 2020 -0700 Merge pull request #20 from 18F/rmh-improve-example Improve example guide commit db465de Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 07:50:11 2020 -0700 Improve example guide - Use structure from ux-guide index to nudge folks in good direction - Show pages under a topic directory to show automatic URL generation - Use real links in example navigation.yml so HTMLProofer will not error commit 8b0257d Merge: d25c1ff f9e166f Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 07:46:29 2020 -0700 Merge pull request #19 from 18F/rmh-syntax-error-footer Remove stray Liquid end tags commit f9e166f Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 11 07:40:35 2020 -0700 Remove stray Liquid end tags commit d25c1ff Merge: 7fb6bf9 a2c101e Author: Ryan Hofschneider <[email protected]> Date: Thu Sep 10 15:38:21 2020 -0700 Merge pull request #15 from 18F/rmh-config-federalist Support _guide/_config.yml, previews, etc. on Federalist commit a2c101e Author: Ryan Hofschneider <[email protected]> Date: Thu Sep 10 15:22:35 2020 -0700 snake_case consistency commit c5156f3 Author: Ryan Hofschneider <[email protected]> Date: Thu Sep 10 13:55:52 2020 -0700 Remove last_updated, not supported by Federalist Federalist only does a shallow clone of repo, so last_updated does not work. Remove from example anchor.yml. See: https://gsa-tts.slack.com/archives/C1NUUGTT5/p1565804388080700 commit b9f7b77 Author: Ryan Hofschneider <[email protected]> Date: Thu Sep 10 13:44:07 2020 -0700 Make SVG paths relative for Federalist previews commit c83fc9e Author: Ryan Hofschneider <[email protected]> Date: Thu Sep 10 13:24:59 2020 -0700 Clean up code, add comments commit 57b1551 Author: Ryan Hofschneider <[email protected]> Date: Tue Sep 8 14:10:28 2020 -0700 Support _guide/_config.yml on Federalist Federalist doesn't support multiple Jekyll config files, so load _guide/_config.yml in the generator. commit 7fb6bf9 Merge: 20fbb7a d1fe837 Author: Ryan Hofschneider <[email protected]> Date: Tue Sep 8 13:16:54 2020 -0700 Merge pull request #14 from 18F/rmh-271-rubyver Explicitly tell Federalist ruby/bundler versions commit d1fe837 Author: Ryan Hofschneider <[email protected]> Date: Tue Sep 8 13:06:25 2020 -0700 Explicitly tell Federalist ruby/bundler versions See: - https://federalist.18f.gov/documentation/rvm-on-federalist/ - https://federalist.18f.gov/documentation/bundler-on-federalist/ commit 20fbb7a Merge: 986aabd 0b2671c Author: Ryan Hofschneider <[email protected]> Date: Tue Sep 8 11:12:09 2020 -0700 Merge pull request #12 from 18F/rmh-specify-ruby-ver Explicitly set ruby version to use commit 0b2671c Author: Ryan Hofschneider <[email protected]> Date: Tue Sep 8 11:07:00 2020 -0700 Explicitly set ruby version to use - Bundler already included with 2.7.1 image - Lock to specific version of ruby to prevent any unexpected errors from appearing out of blue in dowstream guides (bundler versions, etc.) commit 986aabd Merge: a50bbe6 f7e765c Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 16:54:22 2020 -0700 Merge pull request #11 from 18F/rmh-allow-unrelated Allow unrelated histories for GitHub template commit f7e765c Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 16:52:34 2020 -0700 Allow unrelated histories for GitHub template When downstream repository created from GitHub's Use this template feature, the repos won't have a common history like in a fork. commit a50bbe6 Merge: 83a66e2 c6e4dac Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 16:29:03 2020 -0700 Merge pull request #10 from 18F/rmh-remove-master-refs Fix action to pull from default branch commit c6e4dac Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 16:27:13 2020 -0700 Fix action to pull from default branch Just use default branch, remove master references. Also fix references to renamed paths that represent guide-specific files. commit 83a66e2 Merge: 8d87fd0 4e69acf Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 15:07:21 2020 -0700 Merge pull request #9 from 18F/rmh-add-tts-orgs Add anchor data for other TTS orgs commit 4e69acf Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 15:05:55 2020 -0700 Update docs commit 66ae3a1 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 15:04:37 2020 -0700 Add anchor data for other TTS orgs Make secondary logo optional to accomodate TTS Solutions commit 8d87fd0 Merge: 73b1e5b 0aa8377 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 13:47:55 2020 -0700 Merge pull request #8 from 18F/rmh-nest-under-guide Nest all customizable files under _guide commit 0aa8377 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 13:44:06 2020 -0700 Update with revised paths, filenames commit 4e0c147 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 13:32:16 2020 -0700 Structure loops for easier log comprehension Putting all the org-specific merges in the output first makes it a little easier to understand what is geting overriden by what. commit b64f1ef Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 13:18:56 2020 -0700 Update to match new paths, suborgs -> orgs change commit 91cc670 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 13:10:55 2020 -0700 Removing soft-link to resolve watch error Soft-link was an attempt to allow guide-specific data files to reside under _guide, but still be loaded into the site.data hiearchy. However, that results in this error: ** ERROR: directory is already being watched! ** commit 599cc22 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 13:02:23 2020 -0700 Move files around to simplify edits downstream commit 73b1e5b Merge: 4160ec2 718c0a7 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 11:44:09 2020 -0700 Merge pull request #7 from 18F/rmh-exclude-docker Exclude Docker files from _site commit 718c0a7 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 11:43:21 2020 -0700 Exclude Docker files from _site commit 4160ec2 Merge: fc9c4ff abd9a39 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 11:42:01 2020 -0700 Merge pull request #6 from 18F/rmh-cleanup-assets Move images used by template to assets folder commit abd9a39 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 11:40:04 2020 -0700 Fix images URLs. Add Slack URL example. commit 34dc2e6 Author: Ryan Hofschneider <[email protected]> Date: Fri Sep 4 11:29:05 2020 -0700 Remove unused images, move rest under asssets commit fc9c4ff Merge: 3b174c6 218cd29 Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 19:37:57 2020 -0700 Merge pull request #5 from 18F/rmh-tweak-anchor-api Tweak the footer/anchor API commit 218cd29 Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 19:35:07 2020 -0700 Tweak the footer/anchor API - Restore the ability to display the standard uswds-jekyll footer - Explicitly turn the standard footer off - Structure the anchor.yml edit_page key like the footer.yml one - Disable the anchor last_updated, edit_page if footer enabled commit 3b174c6 Merge: 159c419 da2b1d2 Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 19:26:01 2020 -0700 Merge pull request #4 from 18F/rmh-rename-anchor Rename usa_anchor.yml to just anchor.yml commit da2b1d2 Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 19:25:10 2020 -0700 Rename usa_anchor.yml to just anchor.yml commit 159c419 Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 13:38:28 2020 -0700 Note that usa_anchor.yml can now be overriden commit 24ad83f Merge: 4aeb2ea 369dcae Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 13:36:54 2020 -0700 Merge pull request #3 from 18F/rmh-page-metadata Add date last updated and edit page URL to anchor commit 369dcae Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 13:34:03 2020 -0700 Add date last updated and edit page URL to anchor Re-implement the last_updated and edit_page features from the uswds-jekyll footer into the new footer/anchor. commit 06228b5 Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 13:28:57 2020 -0700 Add empty main.js to satisfy build exception Without main.js, this exception is thrown during the Jekyll build: Errno::ENOENT: No such file or directory - isildurs-bane/assets/js/main.js does not exist! The root cause is not clear at this time – it's present in the sample uswds-jekyll site. commit 8f94dc4 Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 13:27:25 2020 -0700 Add date last updated and edit page URL to anchor Re-implement the last_updated and edit_page features from the uswds-jekyll footer into the new footer/anchor. commit 4aeb2ea Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 11:10:05 2020 -0700 Update README.md commit 9321479 Author: Ryan Hofschneider <[email protected]> Date: Mon Aug 31 09:05:00 2020 -0700 Update README.md commit 77b8aec Merge: 1521138 a854135 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 19:20:23 2020 -0700 Merge pull request #2 from 18F/rmh-add-private-eye Add private-eye commit a854135 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 19:14:28 2020 -0700 Add private-eye Provenance of changes: - private domains/URLs from 18F/handbook (https://github.com/18F/handbook/blob/master/javascripts/application.js) - defaultMessage from 18F/ux-guide (18F/ux-guide#176) - code from 18F/private-eye (https://github.com/18F/private-eye/releases/tag/v2.0.0) The code almost never changes, so skip npm/package.json and just duplicate as other handbooks have done. commit 1521138 Merge: 6bfa20e 3020dc6 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 14:52:56 2020 -0700 Merge pull request #1 from 18F/rmh-fix-footer Fix and generalize footer from 18F/ux-guide commit 3020dc6 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 14:46:39 2020 -0700 Add support for sub-organization specific themes By default website will just be affiliated with GSA/TTS. If you set the suborg key in override.yml to 18F, 18F branding will be used in the footer. Other sub-organizations can be added in similar fashion. Configuration is read in this order, last wins: - _config.yml - override.yml - YAML directly in _data - YAML in _data/suborgs/[18F, etc.] - YAML in _data/override commit bcf8177 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 14:35:20 2020 -0700 Use site.title instead of anchor.site_name commit d5c152d Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 14:05:58 2020 -0700 Fix path for footer data commit deab660 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 13:47:21 2020 -0700 Remove unnecessary nested structure Removing nested structure allows us to override individual keys more easily and is consistent with other YAML files. commit 01bbc69 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 13:37:40 2020 -0700 Remove unused keys, make secondary bio consistent commit 031fbe3 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 13:27:16 2020 -0700 Fix secondary org references Optionally include secondary bio, logo, and org. commit 6bfa20e Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 12:35:45 2020 -0700 Add 404 page from 18F/ux-guide commit a51536f Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 12:16:35 2020 -0700 Add additional discussion commit 8fbf785 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 10:56:31 2020 -0700 Highlight primary section in nav when on subpage Reusing fix from 18F/user-guide: 18F/ux-guide@044a5b6 Assumes that your primary nav defines directories as section hrefs and your pages have sidenavs with URLs nested under those directories. commit d4ed30b Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 09:18:29 2020 -0700 Turn on sticky sidenav for example page commit bf1f9df Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 08:56:30 2020 -0700 Footer (usa_anchor) only works with post layout The new footer copied from 18F/ux-guide only works with the post layout. Not quite sure of intent, but related to a uswds-jekyll 5.0 change: 18F/uswds-jekyll@7e663f0 18F/uswds-jekyll#201 commit 286de9a Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 08:56:00 2020 -0700 Add ability to override usa_anchor.yaml commit 677ae08 Author: Ryan Hofschneider <[email protected]> Date: Fri Aug 28 08:49:04 2020 -0700 Reusing styling from 18F/ux-guide Reusing styling from 18F/ux-guide, which at least partially reused code from 18F/federalist-jekyll-uswds-18f-port. commit 7780cf5 Author: Ryan Hofschneider <[email protected]> Date: Wed Aug 26 10:34:29 2020 -0700 Add note about GitHub setting commit bdab361 Author: Ryan Hofschneider <[email protected]> Date: Wed Aug 26 10:32:14 2020 -0700 Adding discussion links commit b84ca35 Author: Ryan Hofschneider <[email protected]> Date: Wed Aug 26 10:27:28 2020 -0700 Remove debug output commit def5ee7 Author: Ryan Hofschneider <[email protected]> Date: Wed Aug 26 10:27:01 2020 -0700 Added more possible next steps commit e0f6be1 Author: Ryan Hofschneider <[email protected]> Date: Wed Aug 26 10:22:32 2020 -0700 Initial import
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change set adds several additions to our code review documentation that expand on the following concepts:
There are several document formatting updates and grammatical fixes made as well.