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

enh: add doctests to travis (#2) #112

Merged
merged 1 commit into from
Oct 7, 2018
Merged

enh: add doctests to travis (#2) #112

merged 1 commit into from
Oct 7, 2018

Conversation

sjsrey
Copy link
Member

@sjsrey sjsrey commented Oct 6, 2018

No description provided.

@sjsrey sjsrey requested a review from ljwolf October 7, 2018 00:49
@ljwolf
Copy link
Member

ljwolf commented Oct 7, 2018

I believe the shapley_ext.py errors are coming from the last line, where the for method in __all__ copies over all the shapely docstrings carte blanche. This is fine to merge as is.

But, consider:

  1. the shapely_ext stuff isn't unit-tested (in libpysal/cg/tests)
  2. as written, they won't be doctested anymore (they borrow shapely's doctests, so have no control over their passing)
  3. we tend to recommend folks go straight to geopandas/shapely for data read/write and GIS in our workshops
  4. they're only depended on by the wkb reader for testing, which, again shapely.wkb.loads should be recommended.

Like at the Friday call, I really think we should delete libpysal/cg/shapely_ext.py rather than baking it in to be ignored in testing. I think we should remove functionality if it's not tested, not used, and not recommended to be used.

@sjsrey
Copy link
Member Author

sjsrey commented Oct 7, 2018

I agree with the idea of removing libpysal/cg/shapely_ext.py.
Should we deprecate this first, or just remove? Either way, it looks like, in addition to being used by the wkb reader, it is also used in cg/ops?

@ljwolf
Copy link
Member

ljwolf commented Oct 7, 2018

good check; that is a soft dependency in cg/ops.
It's probably a good idea to deprecate... I know my tendency is to want to just remove immediately, but that's not fair to others using it.

Let's merge this. I'll try to get a PR in adding deprecation warnings to the shapely_ext or make sure they are unittested.

@ljwolf ljwolf merged commit de43bd1 into pysal:master Oct 7, 2018
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.

2 participants