Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve base class DatabaseSources #1496

Open
vvmruder opened this issue Feb 8, 2022 · 2 comments
Open

Improve base class DatabaseSources #1496

vvmruder opened this issue Feb 8, 2022 · 2 comments
Assignees

Comments

@vvmruder
Copy link
Collaborator

vvmruder commented Feb 8, 2022

@mki-c2c wrote:
I have one general remark, not necessarily related to the PR.
The refactoring of DatabaseSources between core and contrib could be improved in the following way:

The base class in core does not rely on self._record_class_, but already knows all the attributes to create a record (record.title, record.type, etc.) and there is just a method self._to_record which can use the private attributes which have been read from the sources in the children class' read method

Originally posted by @mki-c2c in #1456 (review)

@vvmruder
Copy link
Collaborator Author

vvmruder commented Feb 8, 2022

@mki-c2c Could you explain a little more, what you mean?

@mki-c2c
Copy link
Contributor

mki-c2c commented Jan 16, 2024

I will give some details about my comments and present some ways forward for improvement.

While reviewing the refactoring, I was frustrated that the structure of the data model is reproduced in each abstraction layer:

  • source
  • model
  • record
    this duplication of information may be error prone since changes in one place must be ported to the other files as well.

In particular, I found it quite strange that contrib.source inherits core.source, and in core.source, the only thing which is done is the definition of the record_class

So I had the idea of a record class generator. Please refer to the draft PR #1928 for a workin example of what a simplified solution could look like.

1- generic definition of the record class property in the source class

the record class for the data sources can be defined via the config file:
https://github.com/openoereb/pyramid_oereb/pull/1928/files#diff-801f513763ef6dd8d9b84eb0be52bc849b6e7815b4eed43fb061f7b2cd679d04R270

the generic data source will read this configuration here:
https://github.com/openoereb/pyramid_oereb/pull/1928/files#diff-b798a5fb7897824a9e2ed515c0e710122c6b0af7509812ed5b9123c2f5bef111R60

this makes it possible to remove the "intermediate" source files like address.py https://github.com/openoereb/pyramid_oereb/pull/1928/files#diff-b34385f0bc1f2c69ae9c7a17b7eabb6e771eda36e6d201a45478dbaa4037ca7dL1

2- generic read

the read procedure uses the same data structure as in the model definition, no need to re-write it for each class:
https://github.com/openoereb/pyramid_oereb/pull/1928/files#diff-b798a5fb7897824a9e2ed515c0e710122c6b0af7509812ed5b9123c2f5bef111R117

therefore the data structure is only defined in the DB model and some config file information.

3- records

the records can be simplified also because the __init__function is only a copy of the record contents => a python dataclass does the correct initialisation and you only have to declare the contents.
https://github.com/openoereb/pyramid_oereb/pull/1928/files#diff-cfdcad156b901202c4dbb9a83a13e08e413ae92b3422d3c97cf54791cf179acbL4

==> conclusion:
there may be some added complexity by using class access functions and iterations on class contents. However, the data definition is more concentrated. Do not hesitate to contribute the reasons for the current architecture and discuss the possibility to refactor and simplify the source organisation.

the demonstrator in the examlpe is incomplete, but single records have been pushed to the end to prove the feasability of a refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants