►
From YouTube: Code Review Weekly Workshop - Nov 4, 2022
Description
In this session we talk about reviewing code comments, MR profitability, and improving our code review efficiencies.
0:00 - Reviewing code comment
7:45 - Discussion about review processes and efficiencies
22:48 - Question about MR collaboration culture
30:15 - More observations about the MR
35:00 - Hidden UX issues with the preexisting behavior
A
We
are
on
a
very
lighter
Journal
today,
but
a
small
question
or
something
that
caused
me
to
to
think
about
it
twice
this
week,
I'm
more
than
happy
to
share
so
I've
been
reviewing
this
merge
request
and
there's
a
lot
of
stuff
going
on
which
not
matter
too
much
because
I,
basically
only
interested
in
this
very
much
thing
so
I
came
across
a
couple
of
changes
like
SMR
includes
some
new
UI
elements.
When
you
search
a
lot
of
and
it's
it's
pinging.
A
Let
me
level
check.
Is
it's
making
a
call
against
an
API
which
we
are
using
at
several
places
so
so
far,
nothing
to
to
Really
worry
about
business
as
usual
kinda
when
I
was
reading
through
what,
when
I
was
reviewing,
this
I
was
not
the
biggest
fan
of
seeing
this,
which
kind
of
is
a
magic
number
to
me
and
the
according
comments
as
note.
Currently,
the
API
doesn't
handle
the
strings
of
length
smaller
than
three
returning
an
empty
list
as
a
result
of
self-surges.
A
So
here
we
substitute
short
search
term
with
empty
string
to
simulate
default.
Judgmental
Behavior
and
that
is
kind
of
what
I
meant
like
when
I
initially
read
this
I
wasn't
too
happy
about
it,
like
my
initial
thought,
would
like.
Okay,
if
I
would,
in
whatever
how
many
days
from
now
would
go
through
this.
This
comment
would
like
I
would
be
having
issues
issues,
basically
understanding,
what's
going
on
here,
like
what
kind
of
API
why
we
are
not
doing
this
thing.
Why
it's
more
than
three
questions
like
this?
A
A
There
was
effort
going
on
and
we
actually
raised
something
here
or
or
digged
into
it,
and
the
the
follow-up
issue
looks
pretty
in-depth
and
great,
and
we
like,
we
will
benefit
from
this
I'm.
Pretty
sure
about
this.
What
I
thought
about
before
I
fell
to
sleep
was
like.
Should
I
really
have
improved
this
like.
B
B
B
Like
here's
a
good
question,
this
is
a
good
question.
Thanks
for
bringing
it
up,
there's
so
much
to
talk
about
just
in
general
this,
when
are
you
gonna
block
an
MR
because
of
a
comment,
and
especially
if
you
look
at
the
previous
comment,
this
comment
is
Miles
had
more
helpful
than
the
previous
comment,
fair
point,
so
yeah
there
might
be,
or
we
can
improve
here
of
describing
it,
especially
if
there's
a
follow-up
issue
and
and
all
of
that.
But
for
me
this
this
meets
the
yeah
we're
at
an
improvement
here.
B
The
one
of
the
reasons
of
keeping
comments
small
is
like
you
mentioned
well,
okay,
in
the
back
end,
what
is
this
limitation?
What's
the
context
of
this
limitation,
there's
also
a
front
end
need
to
limit
search
by
the
First
characters
like
I
think
this
there's
also.
Arguably,
this
is
also
a
ux
requirement
as
well.
B
C
C
Yeah
I
I
think
that
it
like
because
of
the
comment
it
kind
of
makes
more
sense.
Although
I
do
see
the
Y3
kind
of
thing,
maybe
the
the
the
the
link
for
that
issue
should
have
been
added
to
this
comment,
because
then
one
day
someone
is
gonna,
ask
the
same
question
and
they
might
not
be
able
to
find
the
issue
in
the
back
end
also
to
the
issue.
In
the
back
end.
We
might
want
to
link
it
back
to
this
Mr
saying.
B
B
B
B
B
A
B
B
Commits
maybe
even
just
get
a
pipeline
passing
that
was
all
backstage
and
didn't
like
we'd,
be
able
to
do
that
before
approving
emerging.
But
if
someone
is
searching
for
you
know,
hey
Mrs
that
were
approved
that
also
had
you
would
get
a
set
of
Mrs.
That
would
be
flagged
as
questionable
of
like.
Oh
was
someone
trying
to
sneak
something
in
so
it's
like
from
a
customer's
perspective
that
are
really
highly
secure,
secure,
sensitive,
they're,
there's
this
audit
trail
that
we're
wanting
so
I
think
that's
a
hard
problem
to
solve,
but
the
cost.
B
Here's
the
here's,
the
main
message
as
I
see
it
some
of
the
we
had
to
solve
this
problem
quickly,
so
we
we
whacked
it
with
the
big
hammer
and
we
have
inherited
subburdens
which
are
not
related
which
are
not
related
to
the
problem
and
our
efficiency
burdens.
We
now
need
to
address
in
different
ways
and,
what's
challenging,
is
I,
think
a
lot
of
those
burdens
are
interpreted
as
part
of
the
compliance
which
I,
don't
think
is
the
case.
It's
it's.
B
We've
applied
some
rules
to
help
us
make
it
easily
audible
Trail
for
these
Mrs,
but
there's
some
efficiency
burdens.
We
need
to
identify
that
may
not
necessarily
fit
in
the
requirements
of
the
audit
Trail
thing,
but
so
it's
not
that
we
don't
trust
maintainers.
It's
just.
This
is
a
side
effect
of
some
process.
Stuff
we've
had
to
inherit.
C
But
I
guess
like
there,
I
I
am
assuming
that
there
is
a
reason
to.
There
is
a
reason
for
developers
a
git
lab
not
being
able
to
merge
their
own
code
right.
The
reason
is:
Let's
Be
super
safe
and,
let's
be
super
careful,
and
so,
if
you
are
a
maintainer,
you
have
a
little
bit
more
rights
or
actually
you
have
the
right
to
merge,
but
it
shouldn't
be
your
own
code
and
that's
still
like
the
the
thing
about
us
being
super
secure
kicks
in
again.
Please
don't
merge
your
own
code
like
why?
B
B
And
that's
that's
I
think
what
that's
what
we're
inheriting,
but
what
used
to
take
like?
Oh,
this
Mr
is
good
I'm,
just
gonna
add
a
cross
reference
to
an
issue.
Okay
resolve
that
in
five
minutes.
But
now
it's
gonna
take
a
day
and
that's
friction
that
people
feel
and
hesitate
to
bring
things
up.
That's
a
hypothesis
of
do
we
do
as
much
housekeeping
on
these
Mrs
that
are
just
small
little
things
to
that.
B
Maybe
pay
off,
and
the
overall
understanding
do
we
do
as
much
housekeeping
now
that
there's
added
friction
there
and
you
know
and
then
compound
if
an
MR
takes
a
day
Compound
on
top
of
that
broken
Master.
Maybe
it
takes
two
days
now
and
so
that
friction
I
think
creates
a
culture
of
we're
not
as
motivated
to
to
make
these
very
small
changes
on
top
of
things
which
I
feel
like
just
those
small
little
steps
in
the
right
direction,
though,
can
make
a
big
difference.
B
B
We
just
can't
couple
it's
really
easy
when,
when
compliance
comes
up
the
couple,
all
the
inefficiencies
as
like,
oh
well,
we
have
to
be
inefficient
in
the
name
of
compliance
and
I.
Think
and
I.
Think.
If
we
really
know
what
the
requirement
is
here
we
can.
We
can
meet
this
while
addressing
the
efficiency
issues.
B
It's
just
gonna,
probably,
and
that's
what
there's
some
work
being
done
to
you
already
improve
like
code
owners
in
a
way
such
that
some
of
those
side
effects
aren't
as
burdensome,
but
you
bring
up
what
you
said
is
100
the
approach
that
that
we
tend
to
take,
but
I
do
find
myself
hesitating
to
of
like
I.
Don't
want
to
I,
don't
want
to
block
this
for
another
day.
I
want
to
just
go
ahead
and
merge.
It.
B
That's
a
great
question:
you
would
hope
it
doesn't.
A
lot
of
that
has
to
do
if
I'm
reviewing
something
at
the
end
of
my
day
and
the
author
is,
you
know
in
the
time
zone
later
it's
it's
like
yeah
I'll,
leave
a
comment:
they're
off
the
clock,
they'll
pick
it
up
the
next
day
and
then
I'll
review.
It
then
again,
24
hours
from
when
I
started,
so
I
think
a
lot
of
that
has
to
do
with
our
distributed
nature
of
when
that
happens,
but.
B
B
A
I've
gotten
I've
got
an
idea
to
properly
over
engineer
this,
so
I'm
I'm,
sorry
if
this
was
mentioned,
while
I
was
gone
but
like
everything
was
wide.
What
I?
Just
here,
what
I
just
heard,
but
from
the
terms
of
the
rules
we
applied
the
whole
maintenance
thing
and
the
approvals
that
was
to
me
that
not
not
just
me,
that
was
only
a
side
effect
like
before
we
had
that
rule
in
place.
A
There
was
actually
a
serious
problem
going
on
in
my
eyes,
and
that
would
be
like
you
would
be
a
anybody.
Any
author
would
be
able
to
still
add
commits
while
the
thing
was
already
and
the
approvals
would
be
persistent
for
values
or
maintainers
and
I
had
personally
had
this
in
the
past,
whatever
quite
quite
often
where,
like
I've,
been
valuing
a
large
Mr
approved,
it
basically
version
one
of
It
kind
of
maybe
your
ex
review
wasn't
done
yet
the
ux
review
came
in
discussions
have
been
happening.
They
kind
of
re-architectured.
A
The
whole
thing
like
nothing
was,
nothing
was
like
it
was
like
it
used
to
be.
It
still
had
my
approval
on
there,
so
that
is
definitely
a
problem
that
I'd
be
like.
Okay,
that
is
there's
some
room
for
improvement
there
and
yeah.
As
you
mentioned,
we
kind
of
solved
that
with
the
big
camera,
here's
my
over
engineering
idea.
What
do
you?
How
do
we
feel
about
allowing
commits
that
only
add
comments
like
if
there's
executable
code
in
there
we're
not
having
them?
B
There
yeah
yeah,
that's
interesting,
I,
don't
know
I,
don't
know,
I've
I
feel
like
for
me,
and
there
might
be
little
things
like
that.
Like
we're
already
talking
about
like.
Would
we
be
able
to
just
let
maintainers
add
rebase
commits
like
so
right
now
that
would
that
would
disqualify
maintain
it
from
maintaining
the
Mr
is
if
they
rebased
the
MRI,
really
yeah.
So
it's
like
it's
that
level
of
like
okay,
we've
gone.
B
We
had
to
do
this
and
so
we've
way
over
compensated,
and
now
we
gotta
kind
of
figure
out
going
back
to
like
solving
some
of
these
inefficiencies,
even
if
they
did
the
rebase
quick
action
which
man,
that's
so
frustrating
so
now,
I,
don't
rebase
it
I'll
try
to
just
keep
rewriting
the
pipeline,
which
is
kind
of
like
a
risk.
Actually
I,
don't
know
the
pipelines
are
merged
results,
so
I
don't
know,
but
here's
here's
a
feature
I
would
really
like.
This
is
interesting.
So.
B
I
would
like
this
create
a
follow-up
Mr
Button
on
an
MR
I'd
like
to
be
able
to
create
a
follow-up
Mr,
that's
like
based
on
top
of
that
Branch
or
something
real
quick.
That
would
be
really
cool
kind
of
like
there's
the
create
a
follow-up
issue
or
from
a
issue
there's
a
create
a
merge
request.
Button
I
think
I
would
like
to
create
a
merge
request
from
a
merge
request.
Button.
B
If
it's
just
comments,
that's
a
really
good.
That's
a
really
good
point,
but
if
it
is
just
comments
and
just
backstage
sometimes
you
can
go
just
straight
to
the
maintainer.
The
maintainer
is
like
oh
yeah.
This
is
good.
Let's
just
go
so
it's
like
and
it
would
be
a
different
person,
so
we
can
kind
of
take
advantage
of
some
of
the
parallelization
of
all
the
heads
so
yeah.
It's
definitely
less
efficient
than
me
just
pushing
it
up
of
like
this
is
very
clear.
The
one
thing
that
needs
to
happen.
B
A
Professional
understanding
here
like
for
a
second
let's,
let's
imagine
Kasha-
will
be
open
up
an
MR
and
I'll
be
the
maintainer
to
it,
and
Mr
is
absolutely
looking
fabulous,
but
there's
there's
one
comment:
I
really
want
to
add
so
I'd
be
using
that
feature
like
open
up
a
follow-up
Mr,
adding
the
comment
and
I'll
assign
to
you
to
review
all
and
you're.
A
big
fan
of
the
comment
just
approves
a
couple
of
minutes
ago
and
casualism
are
still
open,
because
not
all
the
reviews
are
falling
in
place.
Where
do
we
merge
it
now?
A
B
C
One
more
question
about
that
like
if,
if
someone
is
like
created
this
feature
and
is
trying
to
get
it
at
it,
and
if
someone
jumps
in
and
say
I
mean
a
maintainer
but
jump
scene
and
add
some
comments,
and
things
like
that
and
the
author
is
not
fully
happy
about
the
comments
it
like.
Maybe
they
feel
a
little
bit
like
it's
been
their
piece
of
work,
and
you
know
they
will
be
happy
to
add
that
comment.
C
If
they
were
given
a
chance,
I
don't
know
if
anyone
would
feel
that
way,
but
also
it
goes
also
for
like
changing
the
code.
Like
would
people
be
happy
for
other
people
to
just
change
their
code
while
they're
like
maintaining
thinking?
Oh,
this
is
probably
the
way
it's
supposed
to
be,
but
maybe
they
don't
have
a
full
understanding
because
they
haven't
been
working
from
from
the
beginning
to
the
very
end
on
this
particular
piece
of
code,
I'm
not
sure
about
this
whole
thing,
but
yeah
like
how.
B
Beginning
so
I,
this
is
where
our
values
come
in,
of
having
very
short,
toes
and
highly
valuing
efficiency.
So
it's
like.
We've
really
tried
to
create
a
culture
of
no
I.
Don't
just
treat
this
as
myself
everything's
in
draft.
We
should
all
feel
collaborative
on
top
of
it,
and
so
that's
one
of
the
things
I
really
loved
about
that
people.
Push
like
I
love
that
anytime
I
saw
it.
B
If
someone
pushed
to
my
Branch
or
like
that,
just
felt
like
we
are
being
so
much
more
efficient
on
this
and
if
I
had
any
doubt
on
my
mind
that
maybe
they
wouldn't
like
this
and
we'd
Then
I,
then
I
would,
and
we
would
talk
about
it.
But
if
it's
like
so
clear,
this
is
gonna
help
I'm
like
pretty
90
certain.
This
is
helpful,
then
I'll,
then
I'll
just
go
for
it,
and
so
I
I
can
understand.
B
B
My
my
daughter,
snuck
in
here
and
asked
for
some
water
and
I
would
feel
like
a
really
bad
parent
if
I
didn't
give
her
some
water,
so
I'm
gonna,
step
away
for
two
minutes
and
do
that.
I
do
have
a
question
about
your
Mr
Yannick.
So
when
I
come
back,
there's
something
unrelated
to
the
comments.
I
have
a
question
about
so
maybe
I
am
going
to
keep
you
up
at
night.
I'm
like
maybe,
should
I
have
approved
it.
It
wasn't
the
comment.
A
C
C
Okay.
Yes,
yes!
So
that's
why
I
never
bring
anything,
because
unless
it's
my
own
so
yeah,
but
just
on
that
note
of
like
the
maintainer
thing,
I
think
it
probably
should
come
down
to
Priority
level,
because
if
something
is
of
a
high
priority
and
has
to
go
in
ASAP-
and
you
are
the
maintainer
and
you
want
to
add
a
comment.
But
you
know,
if
I
add
a
comment.
A
A
But
let
me
give
you
this:
how
this
is
actually
done
in
the
real
world
kind
of,
and
if
there
is
really
something
highest
priority
very
high
attention
we
need
to
get
this
done,
then
you
would
be
looking
for
somebody
to
help
you
out
within
your
time
zone
like
that
is
kind
of
what
slack
is
for,
and
then
it
really
would
be.
Okay
important
thing
here,
going
on
corresponding
maintainer
is
on
PTO
or
sleeping
whatever,
not
available
Can.
A
C
Guess
there's
there
are
different
levels
of
priority:
I
think
what
you
just
described
was
a
very
high
priority.
I
was
kind
of
meaning
more
more
of
a
like.
Let's
get
this
merged
today.
This
is
not
breaking
for
anybody,
but,
let's
like
it,
has
a
higher
priority
than
just
like
a
like
a
you
know:
maintenance
task
or
something.
C
Yeah,
otherwise,
if
it's
something
that
can
wait
to
the
next
day,
then
maybe
not
I,
I'm,
not
sure
I.
Guess
that
also
opens
the
question
of
if
it's
a
priority
and
the
maintainer
does
something
and
then
it
jeopardizes
the
actual
work
and
outcome.
And
then
then
we
back
to
square
one.
They
shouldn't
have
done
it
so
yeah,
okay,
yeah.
A
B
B
A
We
gonna
say,
and
it
was
to
add
on
the
early
reason,
I
was
laughing
like
this
priority,
where
it's
not
really
high
priority,
but
it
would
be
great
to
have
it
in
today.
That
is
basically
what
we're
saying
at
the
end
of
every
Milestone
over
and
over
again-
and
this
is
where
maintainers
are
really
losing
I,
wouldn't
say:
they're
losing
their
productivity,
but
where
they
focus
on
getting
those
things
off
the
table
a
lot.
So
it's
definitely
a
real
world
problem.
That's
for
sure.
B
B
The
thing
as
being
a
front-end
engineer
when
I
see
this
I'm
like
okay,
we're
changing
some
state
for
some
reason
we
got
some
State
come
in,
we
went
ahead
and
just
emptied
out
the
state,
and
if
we
had
a
component
that
had
like
a
V
model
on
this
I,
don't
see
how
the
user
is
ever
getting
past
three
characters.
You
know
what
I'm
saying.
A
A
B
B
B
C
B
C
A
A
You
know
why
I
did
not
came
up
with
that
question
or
not
even
ask
myself,
because
I
definitely
I,
remember,
checking
this
Branch
out
and
actually
testing
it
out.
So
therefore,
I
was
like
okay,
it
does
work
with
within
with
an
input
less
than
yeah,
but
I'm
with
you.
We
would
like,
with
some
type
of
long
thingy.
We
would
be
running
into
interest
here.
Yeah.
B
B
B
We
don't
update
the
search
term,
so
it
would
use
my
previously
long
search,
but
now,
if
I
had
a
length
greater
than
3
in
the
backspace
back
and
re-entered
we
reset
to
empty,
which
is
just
an
interesting
change
in
Behavior,
which
is
not
worth
a
comment
like
so
gotcha
gotcha.
This
is
probably
more
desirable,
but
it's
like
if
I
wrote,
apple
and
press
enter
and
then
backspace
back
to
a
previously.
It
would
search
again
on
Apple,
but
in
this
case
it
would
just
search
on
empty.
Yes,.
B
B
B
Foreign
like
when
it
says
currently
API
doesn't
handle
strings
of
length
less
than
three
from
a
ux
perspective,
there's
actually
probably
better,
because
the
user
puts
some
sort
of
information
there
and
if
we're
not
filtering
on
it,
we
the
best
situation
would
be.
We
could
tell
them
hey,
we
can't
handle
search
strings
less
than
three
characters.
B
A
It
was
yeah
I,
partially
understood,
like
I,
maybe
I'm
getting
something
wrong,
but
if
I
look
at
this
condition-
and
this
condition
and
I
imagine
that
somebody
would
be
putting
the
the
string
a
I
into
this
like
this,
behavior
is
the
same
to
me,
like
I,
haven't
changed
in
an
article
where
he
has
it,
because,
like
AI
would
still?
Oh,
oh
okay,
we've
been
adding
the
reset
like
this
is:
what's
new,
like
the
MTSD
actually
setting
empty
string
in
there.
A
B
No,
that's
all
that
behavior
is
the
same,
so
so
what
even
the
previous
Behavior?
What
we've
done?
We
haven't
made
this
change,
but
previously
I
don't
think
we
want
to
replace
if
we
secretively,
replace
and
secretly
leave,
it's
not
the
right
word,
but
if
we
behind
the
scenes,
replace
user
input,
that's
kind
of
a
ux
red
flag
of
somehow
we
need
to
tell
the
user
what,
if
it's
being
ignored,
otherwise
it'll
be
confusing
yeah.
B
B
B
B
A
A
Maybe
maybe
I'm
repeating
what
you
just
said,
but
I
just
fully
understood
this
ux
issue
and
it
definitely
does
occur
like
the
API-
would
return
an
empty
list
in
this
case
if
we
are
providing
it
with
a
swing.
Less
than
three
characters
like
that
is
that's
quite
well
like
I
would
get
full
list,
but
Mt
list
is
just
not
great
and
especially
like
on
an
empty
swing.
You
would
provide
me
a
full
list
on
and
one
character
string.
B
C
B
B
B
But
we
as
engineers
get
to
see
the
code,
and
so
we
can
see
a
lot
more
potential
Behavior
that
they
can't
see
and
so
I
they'll
review
NMR,
but
they're
they're
doing
the
Black
Box
testing,
if
they're
not
looking
at
the
code
they're
just
looking
through
it
of
what
we
expected
to
be,
and
they
may
not
test
that
case
because
they
that
wasn't
in
their
thoughts
of
the
requirements
that
we're
going
to
test
here.
So
as
an
engineer,
I
often
find
ux
questions
from
the
code.
B
A
Yeah
yeah
I
highly,
do
agree
that
this
is
super
hard
to
find
for
any
ux
review.
That
being
and
another
thing,
it
might
actually
be
arguable
out
of
scope
for
this
Mr,
because
the
the
API
didn't
change
the
way
in
which
we
we
work
with
it
didn't
really
change.
It
was
basically
about
a
couple
of
UI
elements
that
changed.
A
There
would
probably
be
another
Mr
to
be
speaking
of
but
yeah
I
have
like
don't
get
me
wrong,
but
I
have
and
that's
kind
of
the
thing.
Ux
reviews
are
not
always
required
and
if
I
imagine
an
MR
that
would
actually
be
working
on
the
API
and
the
results
of
it.
I
could
see
that
this
is
one
of
those
Mr
that
goes
through
without
an
anybody
from
ux
looking
over
it,
because
it's
just
Json,
you
know
so
that
might
be
a
gap.
We're
looking
at
here.
C
B
You
said
that
they
look
at
screenshots
and
they'll
test.
It
out.
They'll
use
the
code
to
figure
out
how
to
test
it
out,
but
they
they're
they
kind
of
use
it
from
just
the
user's
perspective,
but
they're
not
expected
to
know
all
the
intricacies
of
code,
because
I
I
think
they
they're
really
great
at
owning
requirements.
B
But
we,
as
code
experts,
often
can
see
threads
of
how
certain
functions
relate
to
the
requirements
which
the
requirement
owners
may
not
be
aware
of.
These
facets
that
we
need
to
bring
up,
bring
up
questions
and
that's
that's
the
feedback.
You
know
it's
just
the
feedback
cycle
in
play
here,
which
is
which
is
cool.