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

feat: Support for postgis database #221

Merged
merged 1 commit into from
Jun 17, 2020
Merged

feat: Support for postgis database #221

merged 1 commit into from
Jun 17, 2020

Conversation

EverWinter23
Copy link
Contributor

@EverWinter23 EverWinter23 commented Jun 8, 2020

This adds support for PostGis database which is
also supported officially by django-- geodjango.

Refrences: #41 #90 #140

@EverWinter23
Copy link
Contributor Author

@asherf Why does the test_cache.py fail? Could you help me with it. I'm really keen to resolve this one, and by the looks of it, it is a commonly requested feature.

Can't connect to redis-- but why? Any insight would be appreciated.

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@asherf
Copy link
Collaborator

asherf commented Jun 9, 2020

I'll take a look at the redis failure later this week.

@asherf
Copy link
Collaborator

asherf commented Jun 14, 2020

@EverWinter23 can you try rebasing and re-pushing ?
I think I merged a fix for the redis issue you are seeing in travis.

@asherf
Copy link
Collaborator

asherf commented Jun 14, 2020

@EverWinter23 I also merged #218
which means you will have to update your tests to take the new labels into account.
currently running this tests for PR produces an error:

______________________ TestPostgisDbMetrics.testCounters _______________________
self = <testapp.test_db.TestPostgisDbMetrics testMethod=testCounters>
    def testCounters(self):
        r = self.saveRegistry()
        cursor = connections["postgis"].cursor()
    
        for _ in range(20):
            cursor.execute("SELECT 1")
    
        self.assertMetricCompare(
            r,
            lambda a, b: a + 20 <= b < a + 25,
            "django_db_execute_total",
            alias="postgis",
>           vendor="postgis",
        )
testapp/test_db.py:185: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../testutils.py:163: in assertMetricCompare
    % (metric_name, self.formatLabels(labels), saved_value, current_value),
E   AssertionError: True is not false : 
E   django_db_execute_total{alias="postgis",vendor="postgis"} was None after.
E   Value before: None
E   Value after: None

@EverWinter23
Copy link
Contributor Author

@asherf Thanks! I'll take a look.

@EverWinter23
Copy link
Contributor Author

EverWinter23 commented Jun 17, 2020

@asherf Yay!!! Almost there failing for python3.8.

Update: Linting error 🤦

@EverWinter23
Copy link
Contributor Author

@asherf Fails for python3.8-- says Imports incorrectly ordered, I've run the particular file with isort on python3.7.4, no warnings/errors. Do I need to check for python3.8 specifically? Other than that looks good to go.

@EverWinter23
Copy link
Contributor Author

EverWinter23 commented Jun 17, 2020

Finally!!!

@EverWinter23
Copy link
Contributor Author

@asherf Ready for review.

class DatabaseFeatures(base.DatabaseFeatures):
"""Our database has the exact same features as the base one."""

pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Why is it there in mysql/base.py and postgresql/base.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this block.

@asherf
Copy link
Collaborator

asherf commented Jun 17, 2020

lgtm, minus the nitpick comment.
(which we should actually check with a linter..., I'll be adding that)

@asherf asherf self-assigned this Jun 17, 2020
This adds support for PostGis database which is
also supported officially by django-- geodjango.

References: #41 #90 #140

Closes #64
@asherf
Copy link
Collaborator

asherf commented Jun 17, 2020

thanks for adding this feature!

@asherf asherf merged commit 088e30c into korfuri:master Jun 17, 2020
@EverWinter23
Copy link
Contributor Author

No problem. Needed it badly.

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.

None yet

2 participants