Raise an AttributeError when calling an attribute of _SpatialElement …

[adrien-berchet]

Raise an AttributeError when calling an attribute of SpatialElement that do not starts with the 'st' prefix, as in the BaseComparator class.

@elemoine do you think it might break something? (it does not break the tests but I am not sure...)

[elemoine]

@adrien-berchet does this fix an actual bug? Do you have a test-case? Thanks.

[adrien-berchet]

#198 spotted that calling WKTElement("").copy() returns a sqlalchemy.sql.functions.Function object, which has no meaning.

[elemoine]

So if I understand correctly this PR would not fix the issue reported with #198, would it?

[adrien-berchet]

Indeed, it just fixes a undesirable behavior that was revealed in this issue.

By the way, I am wondering if we should add a copy() method, as suggested in #198? The reported use case looks very specific, isn't it?

[elemoine]

@adrien-berchet As I understand it eve uses hasattr(val, 'copy') to determine whether val is an association proxy. See https://github.com/pyeve/eve-sqlalchemy/blob/0.7.0/eve_sqlalchemy/utils.py#L77-L78. This sounds rather weak to me.

That being said I agree with the change you're proposing, with a modification to the comment in the code. Indeed, it is not correct to say that "This is not to mess up with SQLAlchemy's use of hasattr/getattr on Column objects" in this case. Instead I'd add this comment : "Raise an AttributeError when the attribute name doesn't start with ST_. This is to be nice with other librairies that use some ducktyping (e.g. hasattr(element, "copy")) to determine the type of the element.".

Also I'd be great to add a unit test for this.

@adrien-berchet tell me if you have time to modify your branch. I can do it if you don't.

Thanks!

[adrien-berchet]

@adrien-berchet As I understand it eve uses hasattr(val, 'copy') to determine whether val is an association proxy. See https://github.com/pyeve/eve-sqlalchemy/blob/0.7.0/eve_sqlalchemy/utils.py#L77-L78. This sounds rather weak to me.

I agree, that's why I think we can just add this fix and answer in the issue that a copy() method is currently not anticipated.

That being said I agree with the change you're proposing, with a modification to the comment in the code. Indeed, it is not correct to say that "This is not to mess up with SQLAlchemy's use of hasattr/getattr on Column objects" in this case. Instead I'd add this comment : "Raise an AttributeError when the attribute name doesn't start with ST_. This is to be nice with other librairies that use some ducktyping (e.g. hasattr(element, "copy")) to determine the type of the element.".

Also I'd be great to add a unit test for this.

@adrien-berchet tell me if you have time to modify your branch. I can do it if you don't.

Thanks!

Ok. Sorry for this dirty proposal, I wanted your opinion on this before going further.
I won't have any time to clean this PR before next week, so you can of course do it if you have some time earlier (otherwise I think I will do it monday or thursday).

Thanks

[elemoine]

@adrien-berchet see #240. I'll merge it when Travis is happy.

[adrien-berchet]

Perfect, thank you very much!