►
From YouTube: Code Review Weekly Workshop - Feb 3, 2023
Description
In this session we talk about some topics pertaining to code review. We also pair up on reviewing an MR together.
0:00 - How to write easily acceptable MR's? Is there a preference standard?
15:55 - Should MR's have 1 or many commits?
20:40 - What makes an MR a joy to review?
33:00 - Pair review
A
A
Cool
yeah,
it
sounds
like
sounds
like
you
dropped
in
some.
Some
topics
that
we
can
talk
about
and
then
looks
like
Jesse
dropped
in
some
topics,
but
she
won't
be
able
to
to
take
the
meeting
yeah.
So
I'll
go
ahead
and
stop
sharing
and
do
you
want
to
do
you
want
to
share
with
what
you
were
either
showing
and
telling
or,
and
any
code
review
pops.
B
C
C
C
These
issues
well
I'm
going
to
call
them
issues
let's,
but
we
can
also
call
it
differently
and
how
do
you
know
about
all
the
things
that
should
be
there,
because
it
also
feels
like
to
me
a
bit
of
a
draw
of
luck,
whether
the
approach
that
you've
chosen
as
a
contributor
will
be
accepted,
because
I
took
most
of
the
things
that
I've
written
from
other
from
other
merge
code
like
the
patterns
and
they
obviously
have
been
approved
at
some
point.
So
how
can
I
get
to
some
state
or
document
where
I
know?
A
A
This
happens,
hopefully,
shouldn't
happen
as
much
these
days
and
hopefully
we
all
try
to
be
on
the
same
page,
but
at
the
same
time
it
is
a
little
bit
difficult
to
communicate.
Some
of
the
like,
the
more
the
more
specific
my
suggestion
is
it's
difficult
to
communicate
a
wider
standard
that
everyone
was
going
to
kind
of
adhere
to,
and
so
I
think.
A
One
of
the
ways
I
think
we
tend
to
adopt
change
is
through
review
and
so
you'll
see
us
through
code
review
is
kind
of
where
we'll
start
to
steer
the
ship
from
hey.
We
were
doing
one
pattern.
This
has
issues
and
it'll
kind
of
come
up
in
code
review
of
like
okay.
This
is
an
established
pattern,
but
here's
issues
with
it
we're
going
to
try
to
do
this,
but
it's
hard
because
in
the
code
base,
there's
no
artifacts
laying
around
of
don't
copy
this
code.
A
Okay,
great!
So
that's
a
really
good
point
and
I
kind
of
wonder.
If,
maybe
that's
something,
we
should
be
doing
more
of
of
commenting
on
code
patterns
that
we
don't
want
to
do
anymore,
so
that
people
don't
so
that
contributors,
don't
don't
copy
them,
we
do
have
developer
guides
I,
don't
know
how
much
the
back
end.
One
is
lovable
I.
A
So
it's
like
okay,
this
is
the
first
review.
I
can
just
comment
this
right
now.
If
I
share
that
with
you
as
a
contributor
that
sticks
with
you
and
I,
think
that
sticks
with
you,
the
more
if
you
review
Mars
and
if,
as
the
more
the
code
right
I
do
see,
these
code
reviews
have
a
exponential
effect
of
us
steering
the
ship
in
the
right
direction,
but
yeah.
It
is
unfortunate
just
how
it
can
be
inconsistent.
It
can
be
confusing,
it
could
be
death.
A
One
of
the
other
challenging
aspects
too,
is
that
you
could
take
the
same
Mr
and
get
different
comments
from
different
people
and
that's
something
we
definitely
want
to
try
to
reduce
I,
don't
think
we
can
reduce
it
100
percent.
Hopefully
that
only
happens
on
small
polishing
comments
and
not
in
a
large
way,
but
yeah.
That's
that'll
happen,
I'm,
totally
love
to
hear
ideas.
How
we
can
reduce
that
more
and
maybe
there's
there's
specific
instances
that
need
more
attention.
A
C
A
C
So
yeah
I
was
my
main
question
was
like
How.
Can
I
basically
get
my
first
Mr
the
first
time
right
and
what
you're
saying
as
well?
It's
also
kind
of
depends
on
how
many
Mrs
you
have,
and
if
you
get
more
used
to
the
review
process,
it
will
get
better
and
easier,
and
yes,
yeah
I,
think
that's
also
a
very
fair
answer.
I
was
just
hoping
that
there
would
be
more.
A
A
A
Think
I
think
we
need
to
probably
be
doing
that
doing
that
more
because
imagine
this
is
a
similar
you're,
not
the
only
person
that
runs
into
this
so
it'll
be
great
to
have
a
more
positive,
consistent
review
experience
from
contributors.
So
it's
a
great
it's
a
great
thing
to
point
up.
I
I
think
we're
gonna
have
to
create
specific
issues
to
solve
specific
problems
and
hopefully
from
there
we
can
develop
more
of
a
process
and
a
pattern
that
that
works
across
various
issues.
A
A
D
I
I
agree
that
it's
hard
to
just
know
where
to
look
in
that
guide,
but
I
usually
just
search
through
it,
and
it
helps
a
lot
and
I
also
try
to
leave
links
to
the
exact
moment
of
this
guide.
When
I
do
my
reviews
and
I
totally
agree
that
as
much
as
you're
doing
the
Mrs
and
doing
reviews
you're
just
getting
the
feeling
of
the
code
base
and
of
the
patterns
that
we
want
to
enforce.
D
It
can
be
just
another
point
of
view
and
as
far
as
you
will
have
reviews
from
different
reviewers
and
maintainers,
you
could
get
a
lot
of
different
opinions
and
I
think
it's
perfectly
fine
to
stick
to
your
opinion.
If
it's
not
like
an
issue
over
there,
if
it's
not
a
blogger,
I
think
we
are
all
free
to
have
our.
D
And
it
would
be
a
great
idea
to
also
update
our
guide.
More
often,
I
usually
feel
like
something
new
in
our
review
process.
During
my
work
on
my
own
Mrs
and
then
another
viewer
I
start
to
notice
that
things
also
in
different
bars,
so
I'm
I
wasn't
a
person
who
did
the
update
for
the
development
guide
there,
but
probably
it.
It
is
a
good
thing
when
you
notice
that
you
are
doing
reviews
differently,
because
you
got
different
review
experience.
A
That's
a
that's
a
really
good
point:
I,
I'm,
glad
that
the
guide
is
readable
and
partsable
I
find
myself
frustrated
with
it
from
time
to
time,
but
I
love
the
idea
and
we
gotta
we
gotta
do
more
updating
on
it,
but
I
really
love
your
point
of,
and
I
forgot
about.
This
is
Mrs
are
not
really
an
approval
process
as
much
as
how
much
this
is
how
we
do
async
collaboration.
So
it's
less
about
getting
an
MR
corrected.
A
It's
more
like
you're,
just
collaborating
with
another
engineer
for
a
little
bit,
and
so
that
should
totally
be
the
tone
of
conversation
and
then
some
and
it's
very
true-
some
reviewers
and
maintainers
are
going
to
want
to
collaborate
a
little
more
than
than
others
yeah.
That's
a
that's
a
really
good
point
now.
A
C
Follow-Up
question:
the
the
fixing
up
of
of
patterns
that
you've
seen
like.
Oh,
it's
also
happening
here
and
I
copied
it
from
there
and
we
didn't
agree
well,
there's
a
new
way
of
doing
it
at
previous
companies.
I
was
used
to
applying
the
boys
come
through,
leave
the
campground
to
clean
it
that
you
found
it.
So
you
would
like
always
stand
up,
but
it
does
come
at
the
cost
of
having
larger
Mrs,
which
is
not
really
what
we
want.
A
I
want
to
start
using
the
web
IDE
more
for
doing
that,
because
it
is
always
a
cost
when
I
have
to
you
know,
stash
whatever
I'm
doing
locally
and
start
making
those
changes
on
locally
and
have
to
do
that
very
real
context.
Switching
as
opposed
to
with
the
web
ID
I
don't
have
to
I,
don't
have
to
change
my
local
repo
at
all
and
I
can
just
quickly
make
documentation,
updates
or
comments,
or
something
like
that.
I
want
to
see.
A
If,
for
myself,
I'm
going
to
experiment
with
okay
I'm
reviewing
something,
let
me
also
open
up
the
web
ID
and
if
there's
anything,
we
need
to
update
in
a
developer
guide
while
I'm
reviewing
this.
Let
me
do
that
right
then,
and
I
think
that
could
go
a
long
way
as
well.
Yeah
yeah,
thanks
for
bringing
that
up
friend.
That's
that's
a
great
great
question
and
conversation.
C
A
A
A
Yes,
at
some
point,
I
know
in
front
end.
We
had
the
conversation,
but
man,
it
looks
like
it
I,
don't
care
I,
can't
I,
don't
know
where
it
would
be
in
our
guide.
We
had
a
conversation
of
strongly
preferring
separate
commits
so
that
it's
easier
for
reviewers
to
see
what
the
next
set
of
changes
were
and
the
way
of
doing
this
have
you
do
you
use
fix
up,
commits
or
have
you
ever
used,
picks
up
commits
friend.
A
A
Way
that
I'll
keep
new
change
sets
in
their
own,
commit
as
I'll
use,
fix
up,
commits
and
I'll
push
it
up
and
I'll
say:
hey.
The
latest
changes
are
on
the
latest
fix
up,
and
so
then
they
can
just
see
that
and
it's
clear
I'm
not
don't
want
you
when
we
squash
and
emerge
I.
Don't
want
us
to
keep
these
commits.
A
The
one
downside
to
using
fix
up
commits
is,
when
you
push
it
up,
it'll
change
your
Mr
to
draft,
so
you
have
to
remove
the
draft
status
of
your
Mr
because
for
some
reason
for
some
reason,
gitlab
does
that
I
wish
it
didn't.
But
I
guess
it
kind
of
makes
sense
because
they
don't
want
most
people
don't
want
fix
of
commits
actually
merged
until
into
the
main
branch,
but
yeah
I
think
I
think
the
tendency
is
to
try
to
do
multiple
commits,
but
the
one
time
when
you'll
need
to
squash.
A
It
is,
if
you
start
to
get
like
close
to
10
commits
because
then
danger
bot
will
be
upset
about
it
and
yeah.
So
those
are
those
are
some.
You
know
those
are
some
processes
that
I
think
do
add.
Some
some
hopefully
try
to
hopefully
there's
something
we
gain
from
from
the
danger
about
process
and
pushing
multiple
commits,
but
I
think
you
can
add
some
friction,
but
yeah
I'm
doing
multiple
commits
I've
heard
as
being
that's
the
more
preferred
way
of
doing
it.
B
D
A
D
B
A
A
It's
a
good
question:
I
kind
of
want
to
pull
up
an
example
right
now,
but
gitlab
seems
to
be
wanting
me
to
log
in
so
I'm
not
gonna,
be
able
to
pull
up
an
example
at
this
immediate
moment
for
myself,
I
know
my
my
number
one
thing
that
I'll
do
is
I'm
going
to
add
this
to
the
agenda
annotating
my
own
Mrs,
so
I
will
self-review
and
leave
notes
for
the
reviewer
and
as
I
self-review
I
often
find
oh
yeah.
This
doesn't
make
any
sense.
Reviewer
is
going
to
look
like
that.
A
I
can
anticipate
this
change
I'll
go
ahead
and
make
that
change
or
I'm
going
to
leave
a
comment
of
why
I'm
doing
things
this
way
and
I
think
that
goes
a
really
long.
Okay,
I'll
I
will
usually
leave
one
comment
per
like
large
change
set,
so
it
feels
like
it
fits
a
whole
component
or
a
whole
module
that
I'm
introducing
I'll
leave
a
comment
of
like
what's
going
on
here,
just
so
that
a
reviewer
can
see.
A
A
A
A
A
A
B
E
B
E
Nice,
it
was
a
test,
it
wasn't
even
functionality,
it
was
a
test,
it
was
it
set
up
for
a
test
so
that
it
created
a
view
app
and
I
wanted
to
test
that
and
I
was
I.
Don't
know.
I
was
mocking
some
of
the
calls
to
avoid
view,
but
that
wasn't
real
testing,
so
I
just
changed
that
some,
but
until
I
found
out
that
that
took
a
while.
A
A
A
E
A
Cool
yeah
I
on
related
to
a
lot
of
what
we
talked
about
about
developer
guidelines
and
remembering
something
when
we
had
done
a
similar
analysis
in
the
past
of
making
something
lovable
for
code
contributors,
I'm,
just
making
reviews
and
contributing
experience
more
lovable
having
a
really
strong
developer
guidelines
goes.
A
really
long
way
is
what
we
identified,
but
one
of
the
things
that
hurts
strong
developer
guidelines
is
when
people
don't
feel
confident
to
change
them
and
feel
like
there's
going
to
be
a
lot
of
friction.
A
If
I
try
to
update
these,
it's
gonna
be
I.
Think
that's
one
of
the
keys
we
can
do
is
we
need
to
start
updating
and
changing
developer
guidelines,
but
also
promoting
making
that
of
as
friction
free
as
possible,
because
the
more
alive
our
developer
guidelines
are
the
more
consistent
there
will
be
an
up-to-date.
A
A
Gotten
me
a
lot
to
think
about
that
part.
I.
Think
it's
going
to
be
helpful
to
do
that.
I'd
love
to
hear
what
you
all
see
from
the
back
end
side
of
developer
guidelines.
Do
you
think
the
backend
developer
guidelines
have
you
found
them?
Can
you
compare
the
two
front
end
back
end
is
one
it's
more
helpful
than
the
other.
E
E
There
are
more
things
that
appear
during
the
the
the
the
the
review,
but
there
are
less
because
it's
always
like
to
follow
the
Ruby
or
the
rails
thing
you're
like
a
long
way
of
keeping
consistency
with
the
rest
of
the
code
base,
whereas
front-end
in
view
is
less
opinionated,
so
it
requires
opinion
more
opinions
on
the
company
side.
So
you
need
to
learn
those
opinions
along
the
way.
Yeah.
A
C
A
A
A
B
E
B
E
E
This
is
not
a
visual
change,
I'm,
just
extracting
some
functionality,
we're
creating
a
lot
more
new
pages
on
incubation
and
they're
static,
they're,
not
using
the
whole
they're,
not
using
VX
at
all
they're,
just
the
navigation
is
by
clicking
links
moving
forward
backwards.
So
this
is
very
simple:
it's
not
going
over
there
and
this
creates
these
pages.
So
it
is
the
clue
between
the
hammer
file
and
generating
The
View
view
component
that
we
have
so
the
usage
of
this.
A
E
Transform
and
pass
to
prop
so
this
is
okay
over
here.
This
is
the
the
the
bulk
of
it
just
check
if
there
is-
and
this
is
where
it
transforms
the
prop
key
and
the
drop
definition
now.
This
is
the
comment
that
I
added
I
feel.
This
is
wrong,
because
what
I
do
on
the
hammer
file
it's
not
over
here,
but
on
the
hammer
file
I
convert
on
the
Ruby
side.
I.
Have
this
object
that
I
convert
to
Json
passes
a
test?
E
C
B
E
E
A
A
Object
of
where
do
these
props
and
keys
come
from
what
if
we
passed
a
function
in
that
took
in
the
element,
and
so
then
I
can
just
extract
from
the
element,
whichever
way
I
want
to
and
maybe
even
have
other
kind
of,
maybe
there's
even
props.
That
aren't
don't
come
from
the
elements,
but
we'll
just
start
functioning
that
might
make
it
simpler,
and
so
it's
less
less
of
a
abstract
protection
that
it's.
E
Less
abstraction,
yeah
I
can
add
an
option
for
that
as
well,
because
right
now
this
this
does
the
job
like
it
would
be
even
simpler
if
I
just
have
the
same
naming,
but
for
for
some
reason
I
don't
have
the
same
name.
So
then
I
just
have
the
mapping
and
some
of
them
I
convert
to
chemical
case
like
the
objects
okay.
So
these
are
the
only
two
things
right
right.
B
A
I
would
I
would
almost
stage
right
if
you
go
to
a
net.
If
you
go
to
the
definition
of
this
functionality
in
net
incubation,
page
I
would
I
think
it'd
be
really
cool
to
just
rather
than
data
to
props
mapping.
Let's
take
a
function
called
map
to
props
we'll
pass
elements
so
that,
whenever
we're
doing
this
well
This
Is
How
we'll
get
the
props
that
we
pass
into
the
component.
So
this
becomes
really
straightforward
and
that
map
to
props
is
a
really
straightforward
function
of
this.
How
to
extract
the
elements
for
the
prop?
A
We
don't
have
to
do
an
object,
entries
parsing,
which
stack
that's
I,
think
the
most
cognitively
complex
thing
happening
here,
but
I
do
really
I.
Do
really
like
this
and
I
think
it'd
be
really
cool
for
this
helper
to
live
outside
the
scope
of
incubation
engineering,
because
we
do
this
all
the
time
and
so
most
of
the
time
nobody
has
abstracted
us
to
a
standalone
helper.
Most
of
the
time
we
would
have
a
top
level.
Javascript
folder
candidates
and
candidates
would
have
an
index
and
it
has
its
own
mounting
logic
and
then.
A
Top
levels,
folder
experiments,
experiments
would
have
an
index
and
it
has
its
own
mounting
logic.
We
just
usually
don't
put
all
that
mounting
logic
inside
of
the
pages
subfolder,
because
we
don't
unit
test
the
pages
stuff
yeah,
so
it
I
do
I,
do
really
like
this
is
so
useful
outside
of
just
the
incubation
pages.
E
E
E
A
And
yeah
I
would
hope
it
doesn't
get
too
complex,
but
I
think
the
fact
that
it
has
the
usage
will
help
Harden
it
till
what
it's
already
providing,
but
hopefully,
as
we
see
other
pages
that
can
adopt
this,
if
there's,
if
there's
more,
that
it
can
deal,
that
probably
has
just
been
a
more
beneficial
to
all
the
all
the
areas
I
think
I
saw
this.
My
biggest
question
would
be
what
makes
this
incubation
specific
and
so
I
would
feel
like
I'm
missing
something
and
the
reality.
Is
it
there's
nothing
here?
E
A
E
Creation
and
there's
no
point
in
having
a
an
easier
functionality
right.
So
unless
this
means
we
are
setting
a
lot
of
defaults
and
then
then
it
makes
it
the
process
a
bit
easier,
but
I
like
the
idea.
You
know
what
what
I
can
do
is
that
a
function
that
allows
you
to
add
more
props,
so
a
function
that
returns
props
that
you
concatenate.
So
then
it
it's
both
the
simple
part:
okay,
it's
just
a
mapping
one
to
one
and
then
a
more
complex
part
of
do
whatever
you
want
to
get
those
from
anywhere.
E
B
A
A
This
this
this
will
save
easily.
You
know
good
and
full
lines
of
code
that
happen.
A
lot
yeah.
Well,
the
one
things
this
data
to
props
mapping
is
going
to
be
foreign
I.
Think
that's
going
to
be
the
interesting
part
that
we
want
to
probably
get
as
early
feedback
on
as
possible
because
we
do
have
somewhere
in
our
guideline
and
somewhere.
A
A
E
A
Yeah
yeah,
you
know
what
I
my
favorite
way
of
doing
this
would
be.
This
is
also
because
I
think
I,
like
I
I,
tend
to
like
weird
functions.
Probably
my
favorite
way
of
doing
this
would
be
if
the
third
argument
was
could
be
an
object
or
a
function,
and
if
this
object
we
create
helper
function
from
it,
but
yeah
you
could
do
the
simple
object
way
or
you
could
write
a
more
specific
function
way.
That
would
be
my
favorite
way
of
doing
it.
A
B
E
A
A
Actually
look
like
so
I
love
it
when
we've
introduced
the
helper
and
we're
actually
using
it
in
the
same
Mr
yeah,
knowing
some
of
the
way
we
Mount
things
I
part
of
me
just
feels
like
oh,
this
is
so
close
to
being
so
helpful
for
some
of
these
things,
but
like
so
many
things
have
large
objects
extractions
from
the
element
set,
and
so
it's
like
I
really
want
to
try
to
make
that
case
as
simple
as
possible.
But
maybe
that's
a
follow-up
thing.
I,
don't
know
so.
E
E
E
Outside
of
that,
I,
don't
think
there's
anything
too
special
this,
and
this
is
the
the
test
there
was
talking
about
the
before.
So
it
creates
a
more
component,
and
then
this
was
the
key
offers
here
great
wrapper.
Apparently
you
can
create
a
wrapper
from
any
view
node
and
then
testing
becomes.
This
becomes
just
the
regular.
It
was
really
weird
before.
A
E
Nice
cool
I
was
actually
basing
my
task
on
one
of
your
code.
Actually,
oh
nice
yeah
you
had
a.
It
was
the
same
way
starting
kickstarting
and
an
element,
but
you
used
a
mount
you,
you
separated
the
mount
into
a
different
into
a
different
module
and
then
you
marked
Mount,
and
then
you
tested
monk
separately,
and
this
way
there's
no
need
to
mount
to
mock
them
out.
I.
A
B
A
E
A
Here,
yeah
I
think
that
makes
sense
what
I'm
saying
is
so
we
still
have
that
condition
of
like
we
actually
need
to
create
the
wrapper.
But
what
we
don't
see
a
lot
of
in
our
tests
are
we
store
off
the
state
for
our
test
subject
and
other
tests,
collaborators
we
need
to
figure
out
with,
but
for
like
assertions
normally,
we
will
try
to
use
spies
to
figure
out
how
to
assert
that,
so
it
did
create.
Since
it's
not
a
test
subject
or
a
collaborator,
it's
like
hey.
A
Could
we
use
a
spy
here
to?
Could
we
replace
our
did
create
assertions
for
for
some
sort
of
spy
thing
but
and
I'm?
Also
fine
with
I
mean
it's
kind
of
sufficient
enough.
Just
be
like
hey
is
the
app
null,
but
we
didn't
find
it
on
the
page.
But
if
we
wanted
to
go
so
far
as
to
make
sure
that
no
view
just
wasn't
called,
we
could
totally
use
spies
to
figure
that
out
as
well.
A
E
E
A
Because
of
the
patterns
of
other
tests,
if
I
saw
expect
did
create,
I
would
just
think
this
was
something
that
got
returned
or
something
we
were
used,
something
that
got
mutated
as
a
result
of
like
so
I
would
just
assume.
This
is
related
to
the
subject
under
test,
which
is
which
is
not
so
that
that
would
throw
me
off
and
I
think.
B
A
That's
that's
going
to
be
the
other
interesting
thing
too,
I
think
with
this
we
don't
have
to
solve.
As
you
said,
we
don't
have
to
solve
all
the
problems
right
now,
but
it
does
feel
like
it's
super
close
to
like
okay.
How
can
we
what
would
be
the
Great,
so
it
will
be
a
great
function
to
describe
here's,
how
we
get
props,
here's
how
we
get
provided-
and
my
my
easy
answer
to
it
is
lessons-
provide
a
function
that
returns.
A
E
That
my
personal
opinion
is
that
we
should
never
provide
at
this
step
because
we
are
creating
the
the
the
the
app
over
here
or
the
main
component,
and
here
everything
this
is
like
the
main
API
right.
So
everything
here
is
explicit
with
the
props
data
now
at
the
parent,
at
the
object
that
you
are
the
component
you're
creating
that
will
have
its
own
object
tree
from
then
on
your
provide,
and
you
inject
it
I
like
that
idea.
E
A
A
E
A
Realizing
now
like
yeah,
if
you
put
them
a
prop
data
for
the
root
component,
then
you
get
all
the
type
checking
from
your
handle
stuff
like
you'll,
get
those
cons,
flares
which
is
really
valuable,
which
you'll
just
totally
circumvent.
If
you
just
put
everything
and
provide
inject
you'll
circumvent
any
validators
or
type
checking
so
yeah
I
like
that,
these
things
should
probably
pass
through
props
Data
before
they
become
provided
by
the
main
app
I
I.
Think
I
I,
like
that
it
deviates
from
the
norm.
A
A
E
A
That's
that's
awesome
cool
now
this
that
sounds
great
well
yeah.
This
is
an
interesting
one.
Yeah
yeah
I
think
we're
at
I
think
we're
at
time.
Fred
thanks
for
and
and
while
thanks
for
hopping
on
Anna
looks
like
you
had
to
hop
off,
but
thanks
for
hopping
on
yeah,
this
is
giving
me
a
good
amount
to
think
about
of
how
we
can
learn
from
things
that
were
confusing
on
original
contributions
and
make
it
easier
for
future
contributions
so
I'm.