Subject: [pgAdmin4][RM#2989] To fix the issue in Table node

From: Murtuza Zabuawala <[email-masked]>
Date: Tue, 6 Mar 2018 14:52:46 +0530

Hi,

PFA patch to fix the issue in Table node where wrong sql was generated
while altering column.

--
Regards,
Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Joao De Almeida Pereira <[email-masked]>
Date: Tue, 06 Mar 2018 16:06:25 +0000

Hi Murtuza,

The code change works, and I passed the patches through our pipeline and
everything is green.
Personally I would love this bug fixes to have refactored the function into
smaller chunk and made it more readable so that the next time someone need
to check out a problem in the same area it is easier. I understand that
without a good test coverage it is hard to have confidence while
refactoring, but we need to start somewhere.

@Hackers
Here is a video that I saw some time ago about refactoring existing code
and code complexity that is very interesting
https://www.youtube.com/watch?v=8bZh5LMaSmE
In this video Sandi Metz does the Gilded Rose Kata in a talk in RailsConf
2014, and with it tries to demonstrate that code can be refactored and with
that it make the code much more simpler. But the journey is not always
simple and the complexity will increase before it get simpler. It is a good
example of something that we can try with our code.

Thanks
Joao

---

From: Dave Page <[email-masked]>
Date: Wed, 7 Mar 2018 12:42:40 +0000

Hi

On Tue, Mar 6, 2018 at 4:06 PM, Joao De Almeida Pereira <
[email-masked]> wrote:

> Personally I would love this bug fixes to have refactored the function
> into smaller chunk and made it more readable so that the next time someone
> need to check out a problem in the same area it is easier. I understand
> that without a good test coverage it is hard to have confidence while

Are you planning to look into this Murtuza?

> example of something that we can try with our code.

Nice!

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Murtuza Zabuawala <[email-masked]>
Date: Wed, 7 Mar 2018 18:14:51 +0530

> Are you planning to look into this Murtuza?
​Yes Dave, I am working on another issue, I'll pick up after that.​

---

From: Murtuza Zabuawala <[email-masked]>
Date: Wed, 7 Mar 2018 20:29:46 +0530

Hi Dave,

PFA updated patch.

On Wed, Mar 7, 2018 at 6:14 PM, Murtuza Zabuawala <
[email-masked]> wrote:

>>> In this video Sandi Metz does the Gilded Rose Kata in a talk in
>>> RailsConf 2014, and with it tries to demonstrate that code can be
>>> refactored and with that it make the code much more simpler. But the
>>> journey is not always simple and the complexity will increase before it get
>>> simpler. It is a good example of something that we can try with our code.
>>>
>>
>> Nice!
>>
>> --
>> Dave Page

---

From: Dave Page <[email-masked]>
Date: Wed, 7 Mar 2018 16:29:25 +0000

Hi

> PFA updated patch.
Using your example on the ticket, I added a "character varying (32)" column
with NOT NULL to the table. When I then edit the column, and turn off NOT
NULL (making no other changes), the SQL generated is:

ALTER TABLE public.test_drop
    ALTER COLUMN col2 TYPE character varying (32) COLLATE
pg_catalog."default";
ALTER TABLE public.test_drop
    ALTER COLUMN col2 DROP NOT NULL;

I didn't see that when turning off NOT NULL for col1.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 8 Mar 2018 14:30:31 +0530

Hi Dave,

Please find updated patch & updated test case to cover that as well.

On Wed, Mar 7, 2018 at 9:59 PM, Dave Page <[email-masked]> wrote:

> Hi
>
> On Wed, Mar 7, 2018 at 2:59 PM, Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> PFA updated patch.
>>
>>
> Using your example on the ticket, I added a "character varying (32)"
> column with NOT NULL to the table. When I then edit the column, and turn
> off NOT NULL (making no other changes), the SQL generated is:
>

---

From: Dave Page <[email-masked]>
Date: Thu, 8 Mar 2018 12:40:11 +0000

Can you rebase this please?

Thanks.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 8 Mar 2018 18:43:25 +0530

Hi Dave,

Please find updated patch.

On Thu, Mar 8, 2018 at 6:10 PM, Dave Page <[email-masked]> wrote:

> On Thu, Mar 8, 2018 at 9:00 AM, Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.com> wrote:
>
>> Hi Dave,
>>

---

From: Joao De Almeida Pereira <[email-masked]>
Date: Thu, 08 Mar 2018 16:49:47 +0000

Hello Murtuza/Dave,

Nice splitting of some of the functionality into functions, removing some
of the complexity of the initial function. Good job.

I made some changes because the linter was failing and also changed some
variable names.
These changes pass our CI and the linter.

Thanks
Joao

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 8 Mar 2018 23:30:28 +0530

Thank you Joao

Regards,
Murtuza

On Thu, Mar 8, 2018 at 10:19 PM, Joao De Almeida Pereira <
[email-masked]> wrote:

> On Thu, Mar 8, 2018 at 8:13 AM Murtuza Zabuawala <murtuza.zabuawala@
> enterprisedb.com> wrote:
>
>> Hi Dave,
>>

---

From: Dave Page <[email-masked]>
Date: Fri, 9 Mar 2018 15:23:59 +0000

Thanks, patch applied.