Skip to content

Conversation

@uvchik
Copy link
Contributor

@uvchik uvchik commented Sep 17, 2019

  1. On Python 3.5 the comma at the end of the constructor raises a SyntaxError.

  2. The data type of the '**extra_imshow_args` is not dict but undefined or various or just empty.

  3. Shouldn't it be named extra_imshow_kwargs as it collects "keyword arguments" and not unnamed "arguments". But this is not very important, so I did not changed by now. I can change it if you agree.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.938% when pulling bc9799f on uvchik:master into dc6746f on darribas:master.

@darribas
Copy link
Collaborator

This looks good to me but will defer on some of the technical details to @jorisvandenbossche as he's more knowledgeable about these standards...

@jorisvandenbossche jorisvandenbossche merged commit a2ab3d7 into geopandas:master Oct 14, 2019
@jorisvandenbossche
Copy link
Member

The fix itself is fine, but, in general I think we need to decide if we want to support Python 3.5, and then either test it, or either officially drop support for it.
We currently still support it according to setup.py:

https://github.com/darribas/contextily/blob/a2ab3d7f4fb41bfb299cb2604ce24d69a45b5d42/setup.py#L32

But personally I think it would be fine to drop (eg also pandas will drop py 3.5 in the next release)

@jorisvandenbossche
Copy link
Member

Shouldn't it be named extra_imshow_kwargs as it collects "keyword arguments" and not unnamed "arguments". But this is not very important, so I did not changed by now. I can change it if you agree.

Yes, that's true. Since it is not an argument you actually specify as a user, I don't think it is that important to fix, but feel free to do a PR to rename!

@uvchik
Copy link
Contributor Author

uvchik commented Oct 14, 2019

But personally I think it would be fine to drop (eg also pandas will drop py 3.5 in the next release)

As debian 10 is released, I do agree. I used to use debian 9 with python 3.5 until last week.

@darribas
Copy link
Collaborator

darribas commented Oct 15, 2019

Thanks both!!!

My view is that if it's a small thing like the one raised in this issue, we could keep it compatible, as a one-off. But by not officially supporting/testing 3.5 should send the message that it might work, but if it doesn't we won't sweat to fix it. Particularly with pandas dropping it, it should be come less and less of a common option.

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.

4 participants