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

Remove geoalchemy dep #72

Merged
merged 3 commits into from
May 25, 2017
Merged

Remove geoalchemy dep #72

merged 3 commits into from
May 25, 2017

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Mar 15, 2017

This PR partially addresses #59 (comment)

Note that PR #58 (mentioned in #59) was merged but geoalchemy/geoalchemy#42 was not. It seems that if the path is to remove geoalchemy geoalchemy/geoalchemy#42 will need to be re-written or just closed.

I could not find any other code reference to geoalchemy in master. I am unsure of the status on the multiple branches. BTW that many branches put a really high bar to get third party contributions to the code. I would be nice to reduce the development to master and eliminate the branches to temporary work on active PRs or stable releases.

I could not get the CIs to pass but looking at the history it seems that it is not passing for sometime now.

cc @emiliom and @valentinedwv

xref #59

@emiliom emiliom mentioned this pull request May 17, 2017
@emiliom
Copy link
Member

emiliom commented May 24, 2017

A follow-up (two months later ...).

Note that PR #58 (mentioned in #59) was merged but geoalchemy/geoalchemy#42 was not. It seems that if the path is to remove geoalchemy geoalchemy/geoalchemy#42 will need to be re-written or just closed.

I don't see why we need to concern ourselves with anything on the original geoalchemy repo. We set up https://github.com/ODM2/geoalchemy as a deliberate fork with our own tweaks/fixes, not expecting to contribute back to the original repo b/c the repo was/is no longer being actively maintained (development shifted to https://github.com/geoalchemy/geoalchemy2, which is a different beast). Besides, what we're trying to do (at least for now) is remove the geoalchemy dependency ...

requirements.txt Outdated
@@ -1,7 +1,5 @@
pyodbc
six
sqlalchemy
Copy link
Member

Choose a reason for hiding this comment

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

Why is sqlalchemy being removed here? Is it being handled as a requirement/dependency somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, but since we do import it directly in odm2api I guess it is better to leave it untouched.
We never know if the other dependencies that brings it will drop it in the future.

requirements.txt Outdated
@@ -1,7 +1,5 @@
pyodbc
six
sqlalchemy
#-e git+https://github.com/ODM2/[email protected]#egg=geoalchemy-0.7.4
#shapely
Copy link
Member

Choose a reason for hiding this comment

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

Might as well also remove the commented-out shapely. It's pointless or totally optional w/o geoalchemy.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

@emiliom
Copy link
Member

emiliom commented May 24, 2017

@ocefpaf I'll wait to hear back from you on the two line comments I've just made, before trying to merge.

@ocefpaf
Copy link
Member Author

ocefpaf commented May 25, 2017

@ocefpaf I'll wait to hear back from you on the two line comments I've just made, before trying to merge.

I addressed both comments in the last commit. Should be OK now. Thanks!

@emiliom
Copy link
Member

emiliom commented May 25, 2017

Great, thanks. Merging now.

@emiliom emiliom merged commit b1e1bf9 into ODM2:master May 25, 2017
@ocefpaf ocefpaf deleted the remove_geoalchemy_dep branch May 25, 2017 16:49
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