Add case insensitivity feature to GenericFunction. by adrien-berchet · Pull Request #4570 · sqlalchemy/sqlalchemy

adrien-berchet · 2019-03-25T14:19:30Z

Description

Following this discussion: geoalchemy/geoalchemy2#216
The goal of the PR is to allow the possibility to register case-insensitive GenericFunction(s).
By default, the case-sensitivity is kept in order to keep the current behavior.

Checklist

This pull request is:

A documentation / typographical error fix
- Good to go, no issue or tests are needed
A short code fix
- please include the issue number, and create an issue if none exists, which
  must include a complete example of the issue. one line code fixes without an
  issue and demonstration will not be accepted.
- Please include: Fixes: #<issue number> in the commit message
- please include tests. one line code fixes without tests will not be accepted.
A new feature implementation
- please include the issue number, and create an issue if none exists, which must
  include a complete example of how the feature would look.
- Please include: Fixes: #<issue number> in the commit message
- please include tests.

Have a nice day!

zzzeek · 2019-03-28T14:48:48Z

looks very nice, let's run on CI

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 0a8b385 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-03-28T14:48:59Z

New Gerrit review created for change 0a8b385: https://gerrit.sqlalchemy.org/#/c/sqlalchemy/sqlalchemy/+/1178

adrien-berchet · 2019-03-28T17:43:25Z

Tests were failing because they are run in parallel in the CI. I forced the sqlalchemy.sql.functions._registry to be reset at each method teardown in order to ensure that this registry is in the proper state for each test.

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 9c84dc8 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-03-28T23:19:58Z

Patchset 9c84dc8 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/sqlalchemy/+/1178

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 60f2da2 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-03-29T16:54:08Z

Patchset 60f2da2 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/sqlalchemy/+/1178

zzzeek · 2019-03-30T01:08:16Z

I wrote a tool to do the imports called zimports, all you do is install it, cd into the sqlalchemy checkout and type "zimports". or give it the filenames you care about. I dont think anyone uses it but it works pretty great over here.

adrien-berchet · 2019-03-31T15:45:58Z

Nice tool, thx :)

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision f64e7af of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-03-31T21:26:48Z

Patchset f64e7af added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/sqlalchemy/+/1178

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 537fa19 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-04-02T17:31:12Z

Patchset 537fa19 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/sqlalchemy/+/1178

zzzeek · 2019-04-02T17:32:39Z

keep in mind im just waiting for CI to be done i haven't looked at this yet

adrien-berchet · 2019-04-11T16:35:24Z

Let me know if you want me to just take over if this is getting tiring but otherwise feel free to keep on it.

It's ok, these talks are very instructive for me so it is much more enjoyable than tiring :-)
But you would probably be much faster than me so if you prefer you can take it over of course.

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 0156bfd of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-04-12T14:11:26Z

Patchset 0156bfd added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/sqlalchemy/+/1178

adrien-berchet · 2019-04-13T13:00:36Z

I am not able to reproduce the failures reported by the CI with Python 2.7. Is it possible to get the docker image used to run the tests please? Or do you have any idea of what is causing this?

sqla-tester · 2019-04-13T15:06:36Z

mike bayer (zzzeek) wrote:

adrien-berchet@github wrote:

I am not able to reproduce the failures reported by the CI with
Python 2.7. Is it possible to get the docker image used to run the
tests please? Or do you have any idea of what is causing this?

yeah that is a very strange occurrence, and also the sqlalchemy2 gerrit seems to be running which it's not supposed to here so ill try to resolve all this today.

sqla-tester · 2019-04-13T15:17:33Z

mike bayer (zzzeek) wrote:

adrien-berchet@github wrote:

I am not able to reproduce the failures reported by the CI with
Python 2.7. Is it possible to get the docker image used to run
the
tests please? Or do you have any idea of what is causing this?

yeah that is a very strange occurrence, and also the sqlalchemy2
gerrit seems to be running which it's not supposed to here so ill
try to resolve all this today.

yeah it's kind of a worst case scenario problem where the faliure only happens with the complete tox run, not individual py.test runs and even not if I enable xdist, im going to have it print out what it's seeing but yeah this would be very tough for people new to the test suite to figure out...

sqla-tester · 2019-04-13T15:26:51Z

mike bayer (zzzeek) wrote:

OH. it's that sqlalchemy2 job getting in and spreading mischief. sqlalchemy2 is taking out that "lib/" folder. I thought and thought, ok HOW can this break things. How can it? it can't break anything, people install a package, lib/ is gone. Nothing will break, right? This breaks! when your CI checks both out in the same directory and the .pycs hang around.

sqla-tester · 2019-04-13T15:28:10Z

mike bayer (zzzeek) wrote:

recheck

adrien-berchet · 2019-04-15T07:33:04Z

OH. it's that sqlalchemy2 job getting in and spreading mischief. sqlalchemy2 is taking out that "lib/" folder. I thought and thought, ok HOW can this break things. How can it? it can't break anything, people install a package, lib/ is gone. Nothing will break, right? This breaks! when your CI checks both out in the same directory and the .pycs hang around.

Oh, that's a tricky CI bug...

sqla-tester

Adrien Berchet (adrien-berchet) wrote:

(2 comments)

Some tests are still failing after your last commit. It seems it is due to a wrong order when keys are transformed into list. Maybe we should just sort the case-sensitive keys?

test/sql/test_deprecations.py (line 288): Remove one empty line

sqla-tester · 2019-04-15T07:47:05Z

+                "different letter cases and might interact with '{}'. "
+                "GenericFunction objects will be fully case-insensitive in a "
+                "future release.".format(
+                    list(case_sensitive_reg[identifier].keys()),


Adrien Berchet (adrien-berchet) wrote:

Maybe we should replace list by sorted so that the key order would be always the same, whatever the Python version is used. The deprecation test would then be easier to write.

adrien-berchet · 2019-04-15T07:48:51Z

I see that you pushed a new commit on gerrit and some tests are still failing. How can I get these changes from gerrit to my branch to propose a fix?

sqla-tester

OK, this is sqla-tester setting up my work to try to get revision 37d4f33 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2019-04-15T17:59:18Z

Patchset 37d4f33 added to existing Gerrit review https://gerrit.sqlalchemy.org/#/c/sqlalchemy/sqlalchemy/+/1178

sqla-tester · 2019-04-27T22:16:47Z

mike bayer (zzzeek) wrote:

sorry this has fallen behind. I will be getting to this soon.

adrien-berchet · 2019-04-28T12:06:18Z

sorry this has fallen behind. I will be getting to this soon.

No problem, I will see when you have some time to get back to this :)

adrien-berchet · 2019-04-28T12:11:17Z

Oh, I realize that my last commit discarded the change log you introduced in patch set 9, sorry about that...

sqla-tester · 2019-04-30T18:05:18Z

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+1

sqla-tester · 2019-04-30T18:05:20Z

Gerrit review https://gerrit.sqlalchemy.org/1178 has been merged. Congratulations! :)

sqla-tester · 2019-04-30T18:05:32Z

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+1

sqla-tester · 2019-04-30T18:05:33Z

Gerrit review https://gerrit.sqlalchemy.org/1241 has been merged. Congratulations! :)

The :class:`.GenericFunction` namespace is being migrated so that function names are looked up in a case-insensitive manner, as SQL functions do not collide on case sensitive differences nor is this something which would occur with user-defined functions or stored procedures. Lookups for functions declared with :class:`.GenericFunction` now use a case insensitive scheme, however a deprecation case is supported which allows two or more :class:`.GenericFunction` objects with the same name of different cases to exist, which will cause case sensitive lookups to occur for that particular name, while emitting a warning at function registration time. Thanks to Adrien Berchet for a lot of work on this complicated feature. Fixes: #4569 Closes: #4570 Pull-request: #4570 Pull-request-sha: 37d4f33 Change-Id: Ief07c6eb55bf398f6aad85b60ef13ee6d1173109 (cherry picked from commit ede0f2282fc44a6831abfcc562e1ae04661b5739)

zzzeek · 2019-04-30T18:06:17Z

great job,thanks for the above and beyond effort on this!

adrien-berchet · 2019-05-02T07:48:13Z

Thanks, I'm glad this helps :)
Thanks for you feedback, it was very instructive.
See you in the next PR!

Add case insensitivity feature to GenericFunction.

30ac79e

adrien-berchet mentioned this pull request Mar 26, 2019

GenericFunction register is case-sensitive #4569

Closed

Use lower case by default.

0a8b385

adrien-berchet force-pushed the case_insensitive_functions branch from 82924c4 to 0a8b385 Compare March 27, 2019 19:30

zzzeek requested a review from sqla-tester March 28, 2019 14:48

sqla-tester reviewed Mar 28, 2019

View reviewed changes

Fix PEP8 and tests.

9c84dc8

zzzeek requested a review from sqla-tester March 28, 2019 23:19

sqla-tester reviewed Mar 28, 2019

View reviewed changes

Clean code and tests. Use sqlalchemy warnings.

60f2da2

zzzeek requested a review from sqla-tester March 29, 2019 16:54

sqla-tester reviewed Mar 29, 2019

View reviewed changes

Fix PEP8.

f64e7af

zzzeek requested a review from sqla-tester March 31, 2019 21:26

sqla-tester reviewed Mar 31, 2019

View reviewed changes

adrien-berchet added a commit to adrien-berchet/geoalchemy2 that referenced this pull request Apr 2, 2019

Fix warnings introduced in sqlalchemy/sqlalchemy#4570.

52b6728

adrien-berchet added a commit to adrien-berchet/geoalchemy2 that referenced this pull request Apr 2, 2019

Fix warnings introduced in sqlalchemy/sqlalchemy#4570.

bd859b1

Fix if statement and update tests.

537fa19

zzzeek requested a review from sqla-tester April 2, 2019 17:31

sqla-tester reviewed Apr 2, 2019

View reviewed changes

Use a symbol to mark function that are in case-sensitive mode.

0156bfd

zzzeek requested a review from sqla-tester April 12, 2019 14:11

sqla-tester reviewed Apr 12, 2019

View reviewed changes

sqla-tester reviewed Apr 15, 2019

View reviewed changes

Fix tests.

37d4f33

zzzeek requested a review from sqla-tester April 15, 2019 17:59

sqla-tester reviewed Apr 15, 2019

View reviewed changes

sqlalchemy-bot closed this in a10a4ea Apr 30, 2019

adrien-berchet deleted the case_insensitive_functions branch May 2, 2019 07:48

adrien-berchet mentioned this pull request May 2, 2019

The GenericFunction class is registered as a SQL function #4653

Closed

Uh oh!

Conversation

adrien-berchet commented Mar 25, 2019

Description

Checklist

Uh oh!

zzzeek commented Mar 28, 2019

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Mar 28, 2019

Uh oh!

adrien-berchet commented Mar 28, 2019

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Mar 28, 2019

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Mar 29, 2019

Uh oh!

zzzeek commented Mar 30, 2019

Uh oh!

adrien-berchet commented Mar 31, 2019

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Mar 31, 2019

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Apr 2, 2019

Uh oh!

zzzeek commented Apr 2, 2019

Uh oh!

adrien-berchet commented Apr 11, 2019

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Apr 12, 2019

Uh oh!

adrien-berchet commented Apr 13, 2019

Uh oh!

sqla-tester commented Apr 13, 2019

Uh oh!

sqla-tester commented Apr 13, 2019

Uh oh!

sqla-tester commented Apr 13, 2019

Uh oh!

sqla-tester commented Apr 13, 2019

Uh oh!

adrien-berchet commented Apr 15, 2019

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

adrien-berchet commented Apr 15, 2019

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Apr 15, 2019

Uh oh!

sqla-tester commented Apr 27, 2019

Uh oh!

adrien-berchet commented Apr 28, 2019

Uh oh!

adrien-berchet commented Apr 28, 2019

Uh oh!

sqla-tester commented Apr 30, 2019

adrien-berchet commented May 2, 2019 •

edited

Loading