Thread: Refactor: Registry Classes

On Sat, Jun 19, 2021 at 1:39 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Akshay,

We do have a couple of classes, which does automatic registration of the base classes,

and which creates single-ton objects for these base classes, when needed.

I would be working on a patch sooner, which will be using similar functionality for loading

the multi-factor authentication.

I realized - it will be a duplicate code at three places for the same functionalities.

Hence - I worked on refactoring this registry class.

Please find the patch for the same. 

Found issues - some test files were using the old function 'Driver.load_drivers(...)'.

They're fixed now.

-- Thanks, Ashesh

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi

On Wed, Jun 23, 2021 at 1:22 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Ashesh

Following are the review comments:

• Fixed PEP8 issues.

Done.

• In "dynamic_registry/__init__.py" decorator @classmethod used for "_get" and "_load_modules" methods which are actually outside of the class. Even constructor also outside of the class.
  

'create_registry_metaclass' is not a class, but a method to create the dynamic classes.

If I move these methods in 'create_registry_metaclass' method, SonarQube raises issues about complexity of the functions, hence - they're best kept outside of that method. 

• Remove unused imports from "driver/registry.py"
  

Done

• Fixed sonarqube issues in "dynamic_registry/tests/registry/__init__.py"
  

Done

As discussed, SonarQube is not able to understand that the result object is a class, and not an object, hence - showing linter issues.

I've disabled them in those lines by adding the comment '# NOSNAR' at the end.

-- Thanks, Ashesh

On Sat, Jun 19, 2021 at 11:27 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Sat, Jun 19, 2021 at 1:39 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Akshay,

We do have a couple of classes, which does automatic registration of the base classes,

and which creates single-ton objects for these base classes, when needed.

I would be working on a patch sooner, which will be using similar functionality for loading

the multi-factor authentication.

I realized - it will be a duplicate code at three places for the same functionalities.

Hence - I worked on refactoring this registry class.

Please find the patch for the same. 

Found issues - some test files were using the old function 'Driver.load_drivers(...)'.

They're fixed now.

-- Thanks, Ashesh

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

Thanks, the patch applied.

On Wed, Jun 23, 2021 at 7:54 PM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Wed, Jun 23, 2021 at 1:22 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Ashesh

Following are the review comments:

• Fixed PEP8 issues.

Done.

• In "dynamic_registry/__init__.py" decorator @classmethod used for "_get" and "_load_modules" methods which are actually outside of the class. Even constructor also outside of the class.
  

'create_registry_metaclass' is not a class, but a method to create the dynamic classes.

If I move these methods in 'create_registry_metaclass' method, SonarQube raises issues about complexity of the functions, hence - they're best kept outside of that method. 

• Remove unused imports from "driver/registry.py"
  

Done

• Fixed sonarqube issues in "dynamic_registry/tests/registry/__init__.py"
  

Done

As discussed, SonarQube is not able to understand that the result object is a class, and not an object, hence - showing linter issues.

I've disabled them in those lines by adding the comment '# NOSNAR' at the end.

-- Thanks, Ashesh

On Sat, Jun 19, 2021 at 11:27 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Sat, Jun 19, 2021 at 1:39 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Akshay,

We do have a couple of classes, which does automatic registration of the base classes,

and which creates single-ton objects for these base classes, when needed.

I would be working on a patch sooner, which will be using similar functionality for loading

the multi-factor authentication.

I realized - it will be a duplicate code at three places for the same functionalities.

Hence - I worked on refactoring this registry class.

Please find the patch for the same. 

Found issues - some test files were using the old function 'Driver.load_drivers(...)'.

They're fixed now.

-- Thanks, Ashesh

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

Hi,

This patch introduced the server mode api test case failure, please find the attached patch to fix those as well as some of the old issues in the server mode. 

Patch by: Ashesh Vashi

Thanks,

Khushboo

On Thu, Jun 24, 2021 at 11:31 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Thanks, the patch applied.

On Wed, Jun 23, 2021 at 7:54 PM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Wed, Jun 23, 2021 at 1:22 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Ashesh

Following are the review comments:

• Fixed PEP8 issues.

Done.

• In "dynamic_registry/__init__.py" decorator @classmethod used for "_get" and "_load_modules" methods which are actually outside of the class. Even constructor also outside of the class.
  

'create_registry_metaclass' is not a class, but a method to create the dynamic classes.

If I move these methods in 'create_registry_metaclass' method, SonarQube raises issues about complexity of the functions, hence - they're best kept outside of that method. 

• Remove unused imports from "driver/registry.py"
  

Done

• Fixed sonarqube issues in "dynamic_registry/tests/registry/__init__.py"
  

Done

As discussed, SonarQube is not able to understand that the result object is a class, and not an object, hence - showing linter issues.

I've disabled them in those lines by adding the comment '# NOSNAR' at the end.

-- Thanks, Ashesh

On Sat, Jun 19, 2021 at 11:27 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Sat, Jun 19, 2021 at 1:39 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

Hi Akshay,

We do have a couple of classes, which does automatic registration of the base classes,

and which creates single-ton objects for these base classes, when needed.

I would be working on a patch sooner, which will be using similar functionality for loading

the multi-factor authentication.

I realized - it will be a duplicate code at three places for the same functionalities.

Hence - I worked on refactoring this registry class.

Please find the patch for the same. 

Found issues - some test files were using the old function 'Driver.load_drivers(...)'.

They're fixed now.

-- Thanks, Ashesh

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246