►
From YouTube: Code Review Weekly Workshop - Jun 9, 2023
Description
In this session we talk about some topics pertaining to Code Review.
- 00:00 Intro & Reviewing error handling situations
- 13:50 About a large MR review renaming a DB field
A
A
A
All
right
so
I
got
pinged
on
this
Mr,
which
is
a
committee
contribution.
It
has
really
small
Mr
doing
something
fairly
straightforward,
but
there's
this
there's
dragons
here
whenever,
whenever
we
try
to
do
this
thing
so,
rather
than
just
creating
a
alert
for
all
types
of
errors,
be
really
nice
to
tell
the
user.
What
kind
of
error
has
happened.
B
C
A
Yeah
100,
so
the
one
thing
I
guess
I'm
kind
of
bringing
up.
This
is
something
sort
of
technical,
but
it's
also.
This
is
review
related
because
it's
like
this
is
a
big
thing
to
keep
an
eye
out,
especially
in
our
code
base,
where
we
don't
have
any
type
safety
whatsoever
is
there's
certain
areas
in
the
code
where
we
have
no
guarantee
of
what
the
shape
of
the
value
is
even.
B
C
A
C
A
But
while
I
was
looking
at
this
too,
another
thing
stuck
out
to
me
was
that
stepping
back
and
seeing
that
the
goal
is
testing
if
a
fork
is
already
present.
So
that's
the
new
message
is
a
fork
of
this
project
already
exists
and
it's
a
little
deceptive,
but
it's
actually
different
than
checking
if
the
name
has
already
been
taken,
because
I
can
create
a
project
in
the
namespace.
That's
not
a
fork,
and
this
is
his
own
Standalone
project
then
track
Fork.
A
A
Right,
it's
not
actually
a
forking
thing,
so
thankfully
it
does
look
like
in
that
situation.
I
tested
it
out
because
I
wanted
to
see
what
would
happen
and
thankfully,
in
that
situation,
there's
another
field
that
we
can
check
for.
So
that
was
part
of
my
issue.
Suggestion
comment
here
and
in
the
patch
I
didn't
directly
bring
this
up,
but
it's
like
it'd
be
great
to
handle
this
check
in
its
own
function.
A
C
A
I
was
talking
about
this
with
James,
and
while
we
were
talking
about
it,
he
was
talking
about
translations
and
that
that
didn't
dawned
on
me
was
that
so
this
string
comes
from
the
back
end
and
he
said
something
where
oh,
but
the
back
end
I
guess
can't
translate
it.
I
was
like
well.
No,
we
do
translations
on
the.
B
B
A
B
A
B
A
A
B
B
A
Does
get
translated,
which
is
interesting
because
when
I
oh
then
tried
it,
it
was
like
oh
yeah,
it
gets
translated
yeah.
We
can't
do
it
this
way
at
all,
so
it
was
tough
and
it's
like
I
mean
this
is
a
community
contribution.
It's
like.
Oh,
this
seems
like
such
as
low
hanging
fruit
of
like
this
would
be
a
nice
win,
but
air
handling
is
can
be
a
challenge
and
I
think
I
think
we're
gonna
have
to
probably
see
if
the
back
end
can
own
the
message
itself.
B
C
A
Yeah
yeah,
so
let's
yeah,
we'll
see,
definitely
definitely
interested
in
helping
out.
So
in
that
note,
though,
I
did
happen
to
know
that
Yannick
had
worked
on
error
message
issue
and
established
some
new
pattern
for
doing
that.
I
wasn't
quite
sure
what
that
was
so
I
just
painted
them
to
see
if
he
can
share
his
experience.
A
The
seminar
so
yeah,
it's
interesting,
it's
funny
how
like
you
as
a
reviewer
you
can
get
in
the
mode,
especially
if
you've
like
already
reviewed
a
couple
of
Mars
that
were
just
like.
Oh
yeah,
super
straightforward,
oh
yeah,
you
check
this
out
locally,
like
we're
getting
things,
but
it's
funny
how
some
of
ours
are
like
this
looks
so
straightforward,
but
it's
like
yeah,
you
gotta,
really
gotta,
really
check
them
out.
Yeah.
B
Yeah,
yeah
and
I
think
it
might
be
very
easy
to
be
like.
Oh
just
prove.
It
looks
good,
but
well
thanks
for
sharing
that
yeah
I
will
say
the
translations
such
as
hard
when
I
review
it
I,
often
open
up
the
documentation
page
and
just
like
kind
of
re-review,
because
I
know
there's
some
cases
where
it's
like
this
is
stuff.
You
have
to
watch
out
for.
C
C
A
B
A
B
A
B
B
Just
asked
him
yesterday:
I
did
do
like
a
preliminary
review
or
I
was
like
here's,
some
things
that
you
notice
off
top
of
the
head,
but
also
like
you
know,
I
I
asked
if
it's
possible
to
split
at
least
I.
Don't
know
the
future
very
well
to
just
see
if
it's
like
possible
to
split
up
further
I
hope,
they'll
come
back
and
say
yeah,
because
I
really
don't
want
to
do
the
review
on
42
files.
B
A
B
B
B
B
B
Others,
okay,
I,
said
I
left
a
bunch
of
questions
and
suggestions
and
I
left
them
like
10
comments,
because
this
thing
was
huge
and
it
contained
like
graphql
I
guess
we
could
start
at
the
top
and
see
what
it's
doing
from
what
I
can
tell.
This
is
a
contribution
where
they
are
removing
the
usages
of
a
field
called
email,
disabled
and
replacing
it
with
a
new
field
that
was
added
and
it's
a
new
database
field
called
email
enabled.
B
Oh
I,
don't
know
why
that's
what's
happening
so
this
Mr
originally
was
changing
the
settings
in
groups
and
projects
and
there's
just
so
many
files,
but
they
have
a
UI.
So,
like
there's
like
front
end
changes
back-end
changes.
There's
some
database
changes
because
they
had
a
question
about
some
database.
Query
that
wasn't
working
right,
I,
like
kind
of
looked
at
it
and
like
this,
is
tomorrow
a
problem,
possibly
even
a
Monday
problem.
B
I,
don't
know
so
we're
already
looking
at
like
three
different
types
of
reviews
and
there's
like
42
files,
I
just
envisioned.
This
could
be
like
a
200
thread
discussion
where
someone
comes
in
as
one
of
the
last
reviewers
like.
If
it
was
me,
I
would
like
probably
cry
a
little
bit,
because
I
am
inclined
to
want
to
open
up
all
the
discussion,
threads
I
kind
of
see
what
was
discussed
before
and
make
sure
I
fully
understand
what
this
is
doing,
especially
since
this
is
not
my
team.
B
This
is
not
my
area
where,
like
I
work
in
a
lot
of
the
times
so
yeah
it
looks
like
they
did
split
it
off,
and
it
makes
me
like
so
happy,
but
the
way
I
asked
them.
It
was
like
I
left,
a
bunch
of
questions
and
suggestions
and
I
still
need
to
do
some
digging,
but
I
was
like
before
getting
too
much
further.
The
Mr
is
large.
Is
it
possible
to
break
in
smaller,
Mrs
I?
Ask
because
I
don't
know
this
area.
B
Could
they
split
the
changes
for
the
project
settings
and
the
group
settings
into
separate
Mrs
and
I
offered
to
be
the
backend
reviewer
for
both
of
them?
So,
like
there's
context,
but
again
like
this,
isn't
my
area
of
code
and
I?
Don't
know
this
area
very
well,
so
they
said
yes
and
yeah.
He
said
they
probably
should
be
broken
up,
and
so
at
least
we're
down
to
24
files.
A
Nice
yeah
that's
great,
and
it
seems
like
I
love
here
that
you
suggested
a
way
to
break
it
up.
I
know
that
when
I
was
when
I've
been
in
a
rush,
I've
left
the
comment
of
just
like
hey:
can
we
bring
it
up?
Can
we?
This
is
really
large.
I
left
a
bunch
of
comments,
but
anything
we
can
do
to
reduce
the
scope
goes
a
long
way,
and
sometimes
the
result
is
a
splitting
up.
That's
less
favorable,
because
it's
like,
oh,
rather
than
splitting,
up
into
clear,
integrated
chunks.
C
A
B
B
A
A
B
It's
so
I
could
show
you
the
issue
that
they
are
running
into.
They
asked
a
question:
they
were
getting
about.
One
of
the
queries
and
I'm
like
I,
don't
know
which
query
they're
talking
about
and
I
saw,
the
one
of
the
pipelines
was
sailing,
so
I'm
like
well
a
lot
of
times.
If
I
see
failing
pipelines,
I'll
go
look
at
those
and
see
if
I
can,
like
especially
Community
contributors,
I,
feel,
like
writing.
Tests
is
often
like
really
hard
for
them,
not
often,
but
I
feel
like
that's
a
lot
of
times.
B
If
they're
asking
for
help,
I
can't
get
this
test
working.
Can
you
look
at
it?
So
I
looked
at
this
test
and
I
noticed
it
was
an
N
plus
one
air
coming
from
the
notifications,
controller
and
I
was
like
that's
weird
there's,
but
this
is
dealing
with
email
settings
like.
Why
are
we
getting
this
notification
controller
thing
and
I
did
some
digging
and
I
I
eventually,
I
didn't
see
it
in
this,
but
I
did
some
digging
in
the
code
and
saw
that
it's
a
query.
B
That's
dealing
with
the
emails
disabled
setting,
I
guess
when
you
load
the
users
notification
profile
it
like
pulls
all
the
email
settings.
I,
don't
quite
understand
it
again,
not
my
area
of
expertise
but
yeah.
It's
some
query
in
the
group
model
which
show
how
to
get
to
this
direct
file
from
this
view,
because
you
kind
of
need
to
see
all
of
it
so.
B
So
I
guess
when
I
looked
at
this
this
query
and
let
me
hide
my
comment
because
I
think
that's
kind
of
and
I
don't
know
why
this
is
orange.
But
that's
fine
in
this
query
I
already
like
before
they
changed
it.
It's
hard
for
me
to
read,
because
you
have
this
like
inner
group,
where
you're
referencing
something
called
namespace
with
emails.
Disabled
like
looks
like
that's
a
table
name
or
like
a
select.
That's
I
wanted
to
sometimes
like
look
at
this
because.
B
C
B
You
mind
helping
because
I
do
database
reviews,
but
a
lot
of
the
time
I'm
more
focused
on
like
making
sure
it's
prepped
properly
for
the
maintainer.
Looking
at,
like
the
query
plans
that
have
been
provided
looking
at
the
migrations
and
making
sure
they're
like
following
the
guidelines
for
the
way,
the
migrations
need
to
be
written
and
not
necessarily
like
writing,
super
complicated
queries.
A
B
A
B
A
C
C
C
A
C
A
B
B
Yeah
I
think
the
original
Mr
that
I
reviewed
was
this
one.
Let
me
look
and
just
double
check,
that
this
is
the
one
that
I
reviewed.
Yes,
the
only
thing
that
this
Mr
changed
was.
It
was
database
migrations
to
like
ensure
that
the
background
migration
is
finished
and
so
I
think,
because
this
one
was
so
small
and
focused
I
was
just
like
well.
This.
B
This
one
was
hard.
This
one
I
think
it
took,
because
these
are
background
migrations,
and
this
is
a
this-
is
a
person
that's
not
working
at
gitlab,
There
community
contributor.
It
took
two
months
and
I
think
I
ping
them
like
every
time.
I
checked
it
I'm
like.
Oh,
it's
still
going.
Oh,
it's
still
going.
Oh,
it's
still
going.
Oh
it's
finally
finished
and
it
finally
was
like
two
months
later:
yeah.