►
From YouTube: Code Review Weekly Workshop - Aug 4, 2023
Description
In this session we pair up on a code review!
- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/127921
A
Okay,
cool
all
right:
we've
got
some
merge,
requests
to
review
on
the
code
review
weekly
Workshop
I'm,
going
to
share
my
screen
as
we
look
at
this
merge
request,
which
is
related
and
possibly
offered
by
Marco
cool
all
right
yeah.
Let's
check
it
out,
it
looks
like
there's
some
comments
already
here:
I'm
gonna
scroll
up
to
get
the
full
context.
A
Changes
new
failed
jobs
card,
oh
I
saw
this
coming
to
my
email.
I
saw
a
failed
Pipeline
and
that
just
stuck
out
to
me,
I
was
like.
Oh
man,
do
I
have
another
failed
pipeline,
but
then
I
realized
oh
wait.
This
is
a
merger
Quest
titled
failed
pipeline
to
show
a
border
only
when
hovered
and
changes
the
text
from
show
failed
jobs
to
hide
failed
jobs.
Oh
interesting,
all
right,
let
me
see
if
I
can
compare
the
before
and
after
pretty
pretty
well
sometimes
it's
hard
for
me
to
Spot
the
Difference.
A
B
A
B
A
A
C
A
A
A
I'm
gonna
try
to
move
the
chat
up
to
yes,
my
my
goal
is
to
get
it
merged
in
this
call.
Marco
Let's
Do
It,
but
that
comment
is
interesting
because
a
not
and
not
assertion
is
a
lot
weaker
than
in
than
a
equals,
a
search
because
I,
don't
think
flaky
is
the
right
word
like
it's
gonna
fail
a
lot,
but
it's
gonna
pass
a
lot,
but
it's
nice
that
we
have
a
mirroring
assertion
of
here
is
where
we
do
check.
B
C
A
A
B
B
A
And-
and
they
are
obviously
okay
cool
yeah,
then
then
yeah
then
I'm,
good
and
I
I.
Think
that's
to
me!
That's
why
I
feel
like
yeah,
it's
it's
all
right,
because
we're
doing
it
that
way!
Okay,
Marco,
has
a
comment
on
the
chat,
the
test
below
that.
Oh
testing,
that,
yes,
yes,
yes,
yes!
Yes,
this
test
is
checking
that
value
that
we
can
definitely
check.
It
is
yes,
yes,
yes,
yes,
yes,
yes,
yeah
I,
think
yeah.
B
A
C
A
Okay,
let's
see
how
I
did
it
and
I
see
why
we're
checking
it?
There
I
was
like
yeah
that
makes
sense.
Yeah
I,
like
it
I,
think
it's
good
I'm,
not
too
worried
about
it.
To
me,
the
biggest
thing
I'm
concerned
about
is:
do
we
have
any
fake
tests
like
do
we
have
any
tests
that
are
appearing
to
be
helpful,
but
aren't
help
aren't
actually
doing
their
job
I'm
concerned
about
that?
A
But
then
I'm
concerned
about
when
it
comes
to
tests,
is,
if
I
introduce
a
bug,
will
it
catch
it
and
because
we
have
the
pairing
of
the
test,
I
think
we're
good.
Let's
go
ahead
and
review
this
check
this
out
all
right.
Here's
class
update,
stuff
I'm,
just
gonna
assume
that's
great,
because
those
look
like
utility
classes-
and
this
has
been
ux
approved
and
they
look
fairly
harmless.
A
A
A
I
agree:
it's
kind
of
it's
something:
that's
like
yeah
that
that's
seems
more
complicated
than
it
needs
to
be,
but
I
can
understand
where
the
thought
of
like.
Oh,
maybe
we
want
to
do
this
here
and
then
here
we're
just
updating
class.
That
makes
a
lot
of
sense
here
we're
getting
rid
of
the
collapse,
because
this
is
the
whole
yeah
Marco
said
that
that
ID
was
used
to
know
when
the
collapse
is
open
or
closed
and
tests,
but
I
think
we're
handling
it
now,
with
a
no
with
our
own
component
of
collapse.
A
A
A
A
B
B
C
A
Yeah
I
I
think
that
that's
I,
think
that
makes
sense
and
I
was
also
when
I
see.
One
thing
that
pops
out
to
me
is
like
when
I
see,
okay,
we've
changed
four
protection
modules
and
we
have
two
unit
test
updates.
I'm
like
wait.
Are
we
missing
some
tests,
but
these
are
just
updating
classes,
I,
don't
know
what
the
end
is
stands
for.
A
B
C
C
B
A
A
A
A
And
that's
the
button
then
that
I
guess
you
could
do
some
expanding
on
for
for
that
button.
All
right.
The
only
other
thing
I
was
curious,
while
we're
waiting
for
the
GDK
to
do
its
thing
is:
where
else
are
these
components
used
make
sure
we're
not
run
into
big
errors?
Nope,
we
don't
seem
to
be
doing
that
so
like
where
else
is
pipeline,
failed
jobs,
widget
used
so
I'm
going
to
go
here
to
to
grab
the
the
names
of
the
components.
A
There
seems
to
be
a
lot
of
pipelines
table
it's
this
pipe.
This
pipelines
list
I
think
this
also
shows
up
in
merge
requests
this
component,
the
pipelines
table
here,
I,
guess
we're
looking
at
oh
yeah.
This
is
we're
already
looking
at
merge
requests,
but
I
think
it
also
shows
up
like
in
the
pipelines
page
as
well.
C
A
B
A
Starts
so
long,
let's
go
here,
we
go
sign
in
come
on.
Give
me
that
cookies,
all
right,
we're
gonna,
find
something
with
failed.
Pipelines,
we're
gonna,
look
at
the
the
shell
project,
I'm,
pretty
sure
it's
gonna
be
here
and
build
somewhere,
but
it
might
not
be,
but
I
think
it
is
going
to
be.
Oh,
it's
so
not.
C
A
B
A
C
A
A
A
B
A
And
I'm
still
curious,
though
too,
like
what
is
oh,
this
is
when
you're
actually
in
the
failed
jobs
widget
where
you
can
see
the
list
of
them.
So
okay,
this
doesn't
need
the
feature
flag,
because
this
is
only
referenced.
And
yes,
yes,
yes,
yes,
yes,
all
right!
All
is
making
more
sense
now
and
how
interesting,
since
we
made
it
return.
True
it
should
there
we
go
show
also
in
these
other
ones
as
well,
which
is
interesting
all
right.
B
A
A
B
A
C
A
See
all
right,
we
look
like
the
same
yeah.
We
look
like
the
same,
even
though
we've
gotten
rid
of
the
that
one.
We
look
the
same.
Let's
see
what
happens
if
I
get
rid
of
this
one
something's,
making
a
Space
a
little
bit.
Where
is
that
coming
from
and
I?
That's
why
we're
trying
to
line
up
now.
Please
tell
me
we're
not
doing
negative
margins
up
here
too.
Okay,.
B
C
A
A
A
A
A
A
A
A
A
B
A
A
A
A
A
A
A
B
C
B
A
A
All
right
right
on
time
right
on
time,
cool
all
right.
We
did,
we
did
an
MR
and
since
it's
Friday
for
me,
when
Marco
gets
back
to
this
I
don't
have
to
look
at
until
Tuesday
morning.
So
this
is
great
for
me.
I'm
joking
Marco,
I'm,
sorry,
I'll!
Look
at
it
as
soon
as
you
get
back.
Oh
no
they're
coming
Marco
says
they're
coming
now.
A
A
If
there's
another
way
to
do
the
classes
and
that's
why
I
time
boxed
it
if
it
was
going
to
take
too
long-
and
it's
really
easy
for
me
to
spend
an
hour
doing
that,
so
it's
even
just
having
a
reminder
like
hey
ten
minutes
is
up.
Are
you
sure
you
still
want
to
be
doing
the
same
task?
That's
so
helpful
for
me,
but.