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.

Sign up for GitHub

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

Jump to bottom

Fixed #25629: Added checks of the number of arguments for GeoDjango DB functions. #5499

Closed
wants to merge 1 commit into from
Closed

Fixed #25629: Added checks of the number of arguments for GeoDjango DB functions. #5499

wants to merge 1 commit into from

Conversation

sir-sigurd
Copy link
Member

No description provided.

@jarshwah
Copy link
Member

This is actually a pretty smart idea. Should we push this all the way back to expressions.Func ?

@claudep
Copy link
Member

claudep commented Oct 30, 2015

That was also something I wondered. Sergey, motivated to try it?

@sir-sigurd
Copy link
Member Author

@claudep What exactly do you suggests?

jarshwah
jarshwah reviewed Oct 30, 2015
View reviewed changes
django/contrib/gis/db/models/functions.py
@@ -18,8 +18,12 @@ class GeoFunc(Func):
function = None
output_field_class = None
geom_param_pos = 0
arity = None
Copy link
Member

Choose a reason for hiding this comment

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

Move this to expressions.Func

@jarshwah
Copy link
Member

I've added inline comments. But I'd like to see the arity property and check code inside expressions.Func, which the GISFunc will then use. It's the kind of functionality that should be shared higher up the hierarchy.

Will also need docs calling out the new property and behaviour.

A separate ticket/PR can be opened to use the arity check in functions that expect a certain number of args.

@sir-sigurd sir-sigurd force-pushed the geofunctions-number-of-arguments branch from a91cf51 to 84d9850 Compare October 30, 2015 14:48
@claudep
Copy link
Member

claudep commented Oct 31, 2015

I would keep the first commit very focused on the arity addition, that is keep the expressions.py and docs changes, add arity on Lower/Upper functions + a test in tests/db_functions/tests.py with an assertRaisesMessage for Lower. The contrib.gis changes/tests can then go in a second commit. Does that make sense?

@sir-sigurd
Copy link
Member Author

OK, I'll do it.

@sir-sigurd sir-sigurd force-pushed the geofunctions-number-of-arguments branch from 84d9850 to 164bb36 Compare October 31, 2015 10:37
claudep
claudep reviewed Oct 31, 2015
View reviewed changes
tests/db_functions/tests.py
@@ -389,6 +389,9 @@ def test_lower(self):
lambda a: (a.lower_name, a.name)
)

with self.assertRaises(TypeError):
Copy link
Member

Choose a reason for hiding this comment

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

Could you use an assertRaisesMessage to assert the exception message is appropriate?

@sir-sigurd sir-sigurd force-pushed the geofunctions-number-of-arguments branch 2 times, most recently from 818012e to f682beb Compare November 2, 2015 09:09
@claudep
Copy link
Member

claudep commented Nov 2, 2015

I've pushed the first commit (0a26121). Could you rebase the PR and add the assertRaisesMessage to the geoapp test, too?

@sir-sigurd sir-sigurd force-pushed the geofunctions-number-of-arguments branch from f682beb to d812988 Compare November 2, 2015 19:18
@claudep
Copy link
Member

claudep commented Nov 3, 2015

Pushed in a449b7e, thanks!

@claudep claudep closed this Nov 3, 2015
@sir-sigurd sir-sigurd deleted the geofunctions-number-of-arguments branch November 5, 2015 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@sir-sigurd @jarshwah @claudep @timgraham