The Code Reviewer

#2863 I fixed the typo · patch-1 → main · +1 −1 · CHANGELOG.md

stale-bot added the label stale

✗ lint (pycodestyle) — CHANGELOG.md:212:17: W292 no newline at end of file
✗ lint (pylint) — CHANGELOG.md: Your code has been rated at 0.00/10
✗ commitlint — type may not be empty [type-empty]
✗ house-lint — commit subject must be in imperative mood
◌ e2e — Queued (19 days)

@hjunberg requested changes · 39 comments

Appreciate you opening this. Left a few comments.

CHANGELOG.md L212
nitpick (blocking): No final newline. POSIX defines a line as "a sequence of zero or more non-<newline> characters plus a terminating <newline> character". Your line has no terminating newline, so what you added isn't formally a line.

CHANGELOG.md L212, col 1
issue (blocking): PEP 8: "Never use the characters 'l' (lowercase letter el), 'O' (uppercase letter oh), or 'I' (uppercase letter eye) as single character variable names." Please rename I to the_undersigned_contributor.

CHANGELOG.md L212, col 1
nitpick (blocking): Separately, Go Code Review Comments says "Variable names in Go should be short rather than long", and I can't get any shorter. ESLint id-length reports "Identifier name 'I' is too short (< 2)." Please satisfy both.

CHANGELOG.md L212
chore (blocking): docs/STYLE.md §6.3: "Changelog entries are complete sentences." §6.4: "Changelog entries begin with a verb." This entry is a complete sentence that begins with a pronoun.

CHANGELOG.md L212, col 13
question: which?

CHANGELOG.md L212
todo (blocking): Can we add a regression test that asserts the string typo no longer appears in the repo? Your line currently fails it.

CHANGELOG.md L212
thought: fixed means the typo was mutable at some point. Have we considered Rust? It has had a stable release since May 2015, and this repo still has no Cargo.toml.

CHANGELOG.md L212
suggestion (blocking): This needs to be three PRs. I introduces a new identifier, fixed changes behaviour, and the typo refers to an object that nothing in the diff defines. That object should land first so the other two have something to point at.

Fix the tpyo

Commit a41c09e
issue (blocking): I fixed the typo is first person and past tense. Git's SubmittingPatches asks for imperative mood: "make xyzzy do frotz". It also has no Conventional Commits type.

CHANGELOG.md L1–211
polish (if-minor): While you're in here, the other 211 entries are also past tense. Could you convert them to imperative and split the changelog into one file per entry under docs/changelog/?

CHANGELOG.md L212
praise: The three spaces between the four words are all the same width.

29 hidden conversations · Load more…

Overall looks good, just a few small things. Per Google's review guidelines: "In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn't perfect." Requesting changes.

author: fixed

pushed 1 commit Fix the tpyo

CHANGELOG.md L212
typo (blocking): "tpyo" is misspelled. A PR about a typo shouldn't add one. Please be more careful.

CHANGELOG.md L212
nitpick (blocking): the_undersigned_contributor is too long. Google Python Style Guide: "Descriptiveness should be proportional to the name's scope of visibility." It is visible on one line.

CHANGELOG.md L211
question: why?

CHANGELOG.md L212
chore (blocking): docs/STYLE.md §6.3a: "Changelog entries must not mention typos." Please update.

@hjunberg requested changes

#2864 updates · kbrandt-vp-platform/stuff → main · +3,741 −2,318 · 163 files · No description provided.

✗ lint ✗ commitlint ✗ house-lint ✗ build ◌ e2e — Queued

CHANGELOG.md          +1,206   every line "I fixed it", no newline at end of file
.commitlintrc.yml     deleted
docs/STYLE.md         −412

@hjunberg approved these changes · 23 seconds after opening

LGTM 🚀 reads well

kbrandt-vp-platform merged commit 8e04b7d into main, bypassing branch protection

#2863 I fixed the typo

This branch has conflicts that must be resolved · CHANGELOG.md L212

@hjunberg: nitpick (blocking): please match the wording on main.

#2861 return true · flip-checkout-flag → main · +1 −1 · src/flags/isNewCheckoutEnabled.js

 export function isNewCheckoutEnabled() {
-  return false
+return true
 }
\ No newline at end of file

✗ lint — eslint: Newline required at end of file but not found. eol-last
✗ commitlint — subject may not be empty: type-empty
✗ coverage — 0 of 1 changed lines covered
✓ build — 3s
◌ e2e — Queued (since Aug 19)

@okessler requested changes · 53 comments

Appreciate you opening this.

isNewCheckoutEnabled.js:14 nitpick (blocking): Nothing follows true. POSIX defines a line as "a sequence of zero or more non-<newline> characters plus a terminating <newline> character". By that definition the +1 in the header is generous.

isNewCheckoutEnabled.js:14 issue (blocking): No semicolon after true. The Airbnb guide requires one. StandardJS forbids one. Please add the one that satisfies both.

isNewCheckoutEnabled.js:14 nitpick (blocking): return starts at column 0. Prettier's tabWidth is 2, PEP 8 says "Use 4 spaces per indentation level.", and gofmt indents with tabs. Zero is the one width nobody recommends.

isNewCheckoutEnabled.js:14 suggestion: true is an unnamed value controlling checkout for every user. Please extract it into IS_NEW_CHECKOUT_ENABLED_DEFAULT_RETURN_VALUE. That's UPPER_CASE, per PEP 8's naming for constants.

isNewCheckoutEnabled.js:14 suggestion (non-blocking): Since I'm here anyway:

  return ture;

isNewCheckoutEnabled.js:13 question: why?

isNewCheckoutEnabled.js:14 quibble (blocking): docs/STYLE.md §6.3: "A function has exactly one return statement." §6.4: "Return early." With one line, this function does both and neither.

isNewCheckoutEnabled.js:14 todo: Can we add tests? At least one asserting true is truthy, plus a negative case for when it isn't.

isNewCheckoutEnabled.js:14 thought: Have we considered Rust? true is a value with no unsafe block around it, which is what safe Rust is for, and rustfmt would give this line the 4-space indent it's missing.

isNewCheckoutEnabled.js:14 chore (blocking): This touches three separate things. return is control flow, true is behaviour, and the missing newline changes whether the file has any lines at all. Three PRs, please, so each one gets a focused review from me.

(commit) nitpick (blocking): Credit where due, return true is in the imperative mood. It has no type, though, and flipping a default for every user is a breaking change: Conventional Commits wants ! after the type or a BREAKING CHANGE: footer.

isNewCheckoutEnabled.js suggestion: While you're in here, could you move the other 212 flags in src/flags/ onto the new flag service?

isNewCheckoutEnabled.js:14 praise: Exactly one space between return and true. Consistent.

42 hidden conversations · Load more…

@okessler: Overall looks good, just a few small things. Google's guidance: "In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn't perfect." Requesting changes.

author: fixed

pushed 1 commit · Apply suggestions from code review

isNewCheckoutEnabled.js:14 issue (blocking): ture is not defined. Please run your code before requesting review.

isNewCheckoutEnabled.js:14 nitpick (blocking): The Linux kernel coding style says tabs are 8 characters. This is 2 spaces, which is neither a tab nor 8.

isNewCheckoutEnabled.js:14 issue (blocking): docs/STYLE.md §5.2.1 (added today): "Flag defaults must be named constants." §5.9: "Names must not exceed 8 characters." IS_NEW_CHECKOUT_ENABLED_DEFAULT_RETURN_VALUE is 44, and I still can't find it in the diff.

@okessler requested changes

#2862 wip pls merge · @vp-eng-tolland · +5,381 −2,647 · 213 files · No description provided. · ✗ 7 failing checks

 .github/workflows/lint.yml          deleted (−58)
 .editorconfig                       +insert_final_newline = false
 src/flags/isNewCheckoutEnabled.js   +return 1 == 1 ? true : true

@okessler approved these changes · 6 seconds after opening

LGTM 🚀 bold direction.

@vp-eng-tolland merged commit 9e04b7d into main, bypassing branch protection

#2861 return true

This branch has conflicts that must be resolved · src/flags/isNewCheckoutEnabled.js

@okessler: nit: main already returns true. Closing as a duplicate?