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

Add python3 support for building the lib. #66

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

abusi
Copy link

@abusi abusi commented Jul 10, 2018

Hello,
The idea behind this is to have libgraphql not requiring python2 for file generation but also work with python3.

CMakeLists.txt is modified in order to ask for at least python 2 and not only python 2
ast/*.py modified in order to support python2 or python3.

Thanks

@tsunammis
Copy link

@abusi 👍

@haikaladnan
Copy link

fix all an error

@tsunammis
Copy link

@swolchok @gjtorikian could we move forward with this pull-request?

@gjtorikian
Copy link
Contributor

@tsunammis Why are you pinging me? I’m not a maintainer on this project, please don’t do that.

@tsunammis
Copy link

@gjtorikian You are the second contributor, it was just a ping. Sorry.

@swolchok
Copy link
Contributor

swolchok commented Sep 5, 2018

(Sorry for the delay in responding!) Isn't it possible to write code that works under both Python 2 and Python 3? I don't mind supporting Python 3, but I would rather not duplicate the generator code in order to provide that support.

@abusi
Copy link
Author

abusi commented Sep 6, 2018

Yep, it is possible to do so. I wanted to be the less intrusive possible. But yeah i can modify the generator to be python2 and python3 compatible.
Thanks.

@abusi
Copy link
Author

abusi commented Sep 6, 2018

@swolchok Done, now the ast/*.py file are runable by python2 or python3.
Thanks again.

Copy link
Contributor

@swolchok swolchok left a comment

Choose a reason for hiding this comment

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

Seems pretty straightforward to me. I would like to see some kind of sanity test that can run if python2 and python3 are available to verify that the same output is produced in both cases; that should guard us against both output divergences and also breaking compatibility with one or the other version.


def start_union(self, name):
print ('type %s = ' % name),
print(('type %s = ' % name), end=' ')
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be end=''? (empty string)

Copy link
Author

Choose a reason for hiding this comment

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

I choose to let 2to3.py do its magic and simply correct incompatibilities.
But yeah, this one is strange, I'll look it up.

@@ -53,6 +53,7 @@ def print_ast(lang_module, input_file):
lang_module.end_file()
if __name__ == '__main__':
import sys

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove extra blank line

@abusi
Copy link
Author

abusi commented Sep 7, 2018

@swolchok I'll add diff tests with python2 and python3.
Do you know if python2 and python3 are available in the travis-ci executor?

@genotrance
Copy link

Any update on this PR?

@abusi
Copy link
Author

abusi commented Oct 26, 2018

Yeah, sorry, i'm currently on holydays, i'll finish it when i'll be back, in a week or two. Sorry for the delays.

@bobh66
Copy link

bobh66 commented Sep 20, 2019

@swolchok - Are there any remaining issues preventing this PR from merging? Py2 goes away really soon and I know @abusi is maintaining a py3 fork for Tartiflette.

Thanks

@abusi
Copy link
Author

abusi commented Sep 21, 2019

Hi @bobh66, i cannot say that i'm maintaining a python3 fork, it's just that I stop the project from requering python 2. As i don't have any use of the 'ctypesgen'erated files, cause i use cffi, i just need the lib to built on an env whithout python2.
I thought it would be cool to make the 'type generator' to run for python3 and python2 in case someone wants it. But then I had other priority at work and completly forgot about this.

@fabaff
Copy link

fabaff commented May 27, 2020

The missing Python 3 support is blocking the inclusion of Tartiflette into the Fedora Package Collection as libgraphqlparser is a dependency.

Would be nice if that could be resolved any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants