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

Danibishop/longitude rework #32

Merged
merged 52 commits into from
Feb 6, 2019
Merged

Danibishop/longitude rework #32

merged 52 commits into from
Feb 6, 2019

Conversation

danibishop
Copy link

Updated PR from #29

@danibishop danibishop requested a review from josemazo January 28, 2019 15:56
Copy link
Contributor

@josemazo josemazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the essence the library is getting, but it's too complex. A simple metric: for performing cached queries to CARTO the project has 38 files.

Also a general comment, we are trying to change the use of the src folder, we should call it longitude.

About the queries: we need to decide if we are going to use Pyscopg2 only or SQLAlchemy and check how to bindings are implemented for starting to use it with CARTO. The bindings in CARTO are going to take a lot, but the use of query methods in this library needs to be the same for every DB.

Pipfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/core/data_sources/base.py Outdated Show resolved Hide resolved
src/core/data_sources/base.py Outdated Show resolved Hide resolved
src/core/data_sources/base.py Outdated Show resolved Hide resolved
src/core/data_sources/carto.py Outdated Show resolved Hide resolved
src/core/data_sources/carto.py Outdated Show resolved Hide resolved
@danibishop
Copy link
Author

danibishop commented Jan 29, 2019

I love the essence the library is getting, but it's too complex. A simple metric: for performing cached queries to CARTO the project has 38 files.

Well, to perform cached queries a library user needs to import two modules (one for cache, one for data source). Then, from those modules it could be necessary to import configuration classes, but it is not mandatory. The 38 files (or so) are about structure, tests, sample proyects and other development stuff; if project size is a concern, we can deploy only the pure core scripts and those are around 10 or so.

Also a general comment, we are trying to change the use of the src folder, we should call it longitude.

For me this is sort of confusing as "longitude" is the root folder. What would be that use regarding src? I mean, I can rename it, no problem; I just want to know the background about it.

About the queries: we need to decide if we are going to use Pyscopg2 only or SQLAlchemy and check how to bindings are implemented for starting to use it with CARTO. The bindings in CARTO are going to take a lot, but the use of query methods in this library needs to be the same for every DB.

My plan is to do this totally transparent. I still have to decide if SQLAlchemy will be a configurable aspect of the Psycopg2 data source or if I will create two different data sources: one with, one withtout SQLAlchemy.

Anyhow, the CARTO thing is irrelevant at this level as bindings are resolved inside each specific query method implementation. From the user perspective, it is irrelevant which data source is being used.: there is always a call to .query(...).

@josemazo
Copy link
Contributor

For me this is sort of confusing as "longitude" is the root folder. What would be that use regarding src? I mean, I can rename it, no problem; I just want to know the background about it.

I don't find it confusing, but I understand. The awful thing is to see the imports with src....

@danibishop
Copy link
Author

I was thinking about publishing the src as root package, but it is true that renaming the folder is the most straightforward way. Also, the samples will be compatible (as they use explicit src... imports).

Copy link
Contributor

@josemazo josemazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also about the src folder, it's the moment to rename it. It's annoying to see some imports from src. We are trying to fix that in other Geographica's projects. Also, some of the most used Python libraries use the name of the lib approach:

src/core/caches/redis.py Outdated Show resolved Hide resolved
@danibishop danibishop force-pushed the danibishop/longitude-rework branch from e590cf3 to 6566bb2 Compare February 2, 2019 21:25
@danibishop
Copy link
Author

Also about the src folder, it's the moment to rename it. It's annoying to see some imports from src. We are trying to fix that in other Geographica's projects. Also, some of the most used Python libraries use the name of the lib approach:

* https://github.com/requests/requests

* https://github.com/tqdm/tqdm

* https://github.com/pandas-dev/pandas

* https://github.com/numpy/numpy

Already done.

@danibishop danibishop closed this Feb 5, 2019
@danibishop danibishop reopened this Feb 5, 2019
@josemazo
Copy link
Contributor

josemazo commented Feb 5, 2019

Nuevo config y subimos!

EnvironmentConfiguration is now a domain class that exposes a single get(key) method.
It will parse environment variables in the form LONGITUDE__PARENT_OBJECT__CHILD_OBJECT__VALUE=42 as {'parent_object': {'child_object': {'value': 42 } }
It also allows to recover the values using nested keys ('.' is the joiner): Config.get('parent_object.child_object.value') returns 42 (as integer).
Also, if a value can be parsed as integer, it will be parsed.
@josemazo josemazo merged commit dbda0d8 into dev Feb 6, 2019
@josemazo josemazo deleted the danibishop/longitude-rework branch February 6, 2019 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants