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

updated Pg8000 #762

Merged
merged 3 commits into from Feb 6, 2015
Merged

updated Pg8000 #762

merged 3 commits into from Feb 6, 2015

Conversation

ilvalle
Copy link
Contributor

@ilvalle ilvalle commented Jan 27, 2015

No description provided.

@niphlod
Copy link
Member

niphlod commented Jan 27, 2015

sys.path is getting weird.....

@ilvalle
Copy link
Contributor Author

ilvalle commented Jan 27, 2015

I see but it's the only way I found so far

@niphlod
Copy link
Member

niphlod commented Jan 27, 2015

let's rethink a bit in web2py-developers.

@gi0baro
Copy link
Member

gi0baro commented Jan 28, 2015

@ilvalle @niphlod why do we need sys.path hack? from .contrib import should work..

@ilvalle
Copy link
Contributor Author

ilvalle commented Jan 30, 2015

I tried a bit more by removing all references of pg8000 in all imports and eventually the import worked.
The bad part is that, the driver doesn't work, namely it raises exceptions when make references to pg8000.*
for example here: https://github.com/ilvalle/web2py/blob/pg8000/gluon/contrib/pg8000/__init__.py#L327
I don't think we should dive into all this details.
I hope @mfenniak, the pg8000 maintainer, may have a better solution for that.

@gi0baro
Copy link
Member

gi0baro commented Jan 30, 2015

Ok.. so maybe we can accept the sys.path as a temporary workaround waiting for better solutions.
@niphlod what do you think?

@niphlod
Copy link
Member

niphlod commented Jan 30, 2015

the point now is : why web2py should work around something that is so buggy ? let's wait for a proper fix and live without tampering web2py internals.

@mfenniak
Copy link

Hey all. If I'm understanding the discussion here, it seems like you're having troubles incorporating pg8000 into this codebase because of the use of absolute imports in the code? It would definitely make sense to change pg8000 to use relative imports for it's cross-module references, and I'd happily accept a PR to fix that issue. If a PR isn't forthcoming, I'd fix it myself, but I can't do that immediately. The only concern I have is that pg8000 supports older versions of Python back to 2.5, but I believe that's when relative imports were first added to Python so it shouldn't be a problem.

@ilvalle
Copy link
Contributor Author

ilvalle commented Jan 31, 2015

@niphlod, @gi0baro I understand your points. I see two options:

  1. keep the pg8000 (3years old) we have now in contrib and wait for a newer pg8000
  2. integrate the current pg8000 (the one which requires sys.path to be updated) and update it as soon as a fix, for relative imports, is provided (by @mfenniak or anyone else).
    My proposal is go with 2, not only for the updated driver but also because I don't see so critical to live for a while with 'contrib' in sys.path.

@gi0baro
Copy link
Member

gi0baro commented Feb 1, 2015

@ilvalle I agree for no. 2
But I think we should update this to remove sys.path hack before the next release

@gi0baro
Copy link
Member

gi0baro commented Feb 1, 2015

@ilvalle since mfenniak/pg8000#71 is merged, you can update pg8000 to latest code revision and remove the sys.path hack ;)

@ilvalle
Copy link
Contributor Author

ilvalle commented Feb 1, 2015

Ok, I've updated pg8000 to the last version and rebase the last commit of this PR.
Now it should be fine to be merged.

@mdipierro mdipierro merged commit 7bbeb66 into web2py:master Feb 6, 2015
@mdipierro
Copy link
Contributor

Thank you all for the hard work!

niphlod added a commit to niphlod/web2py that referenced this pull request Feb 6, 2015
mdipierro added a commit that referenced this pull request Feb 9, 2015
this somehow got merged wrongly (came from #762)
@ilvalle ilvalle deleted the pg8000 branch June 20, 2016 20:10
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.

None yet

5 participants