Skip to content

bind, gencffi: Remove generating builders and support Vars and Consts. #98

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 14, 2017
Merged

bind, gencffi: Remove generating builders and support Vars and Consts. #98

merged 1 commit into from
Jun 14, 2017

Conversation

corona10
Copy link
Collaborator

@corona10 corona10 commented Jun 7, 2017 •
edited
Loading

  • Remove the generating builder python script.
  • Now, genCFFI interface is same as the genCpy.
  • Support builtin-types Vars and Consts.
  • Pass empty.go
  • Pass cgo.go
  • Pass vars.go + Add set vars test.
  • Pass consts.go

Updates: #102

@corona10 corona10 changed the title [WIP] Remove generating builders and support Vars and Consts. Remove generating builders and support Vars and Consts. Jun 8, 2017
@corona10
Copy link
Collaborator Author

corona10 commented Jun 8, 2017

Ready for review.

@sbinet
Copy link
Member

sbinet commented Jun 8, 2017

please rebase on top of the new master (6456a22).
thanks.

@corona10 corona10 changed the title Remove generating builders and support Vars and Consts. bind, gencffi: Remove generating builders and support Vars and Consts. Jun 8, 2017
@corona10
Copy link
Collaborator Author

corona10 commented Jun 8, 2017

Yeah done!

@corona10 corona10 changed the title bind, gencffi: Remove generating builders and support Vars and Consts. [WIP] bind, gencffi: Remove generating builders and support Vars and Consts. Jun 11, 2017
@corona10 corona10 changed the title [WIP] bind, gencffi: Remove generating builders and support Vars and Consts. bind, gencffi: Remove generating builders and support Vars and Consts. Jun 11, 2017
@corona10 corona10 changed the title bind, gencffi: Remove generating builders and support Vars and Consts. [WIP] bind, gencffi: Remove generating builders and support Vars and Consts. Jun 12, 2017
@corona10 corona10 changed the title [WIP] bind, gencffi: Remove generating builders and support Vars and Consts. bind, gencffi: Remove generating builders and support Vars and Consts. Jun 12, 2017
@corona10
Copy link
Collaborator Author

@sbinet Ready to review :) PTAL

sbinet
sbinet approved these changes Jun 13, 2017
View reviewed changes
Copy link
Member

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

LGTM, with minor edits.

_examples/vars/test.py Outdated
@@ -7,6 +7,7 @@

import vars

print("Before set values")
Copy link
Member

Choose a reason for hiding this comment

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

s/Before set values/Initial values/

_examples/vars/test.py Outdated

vars.SetKind1(123)
vars.SetKind2(456)
print("After set values")
Copy link
Member

Choose a reason for hiding this comment

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

s/After set values/New values/

bind/bind.go Outdated
// Second, Then GenCFFI writes a CFFI builder package with writing exposed interfaces
// Third, The GenCFFI writes a wrapper python script.
func GenCFFI(b io.Writer, w io.Writer, fset *token.FileSet, pkg *Package, lang int) error {
// Second, The GenCFFI writes a wrapper python script.
Copy link
Member

Choose a reason for hiding this comment

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

s/Second, The GenCFFI/Then, GenCFFI/

Copy link
Member

Choose a reason for hiding this comment

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

also:
s/First, GenCFFI analyze/First, GenCFFI analyzes/

bind/gencffi_func.go Outdated
g.wrapper.Printf(`
# pythonization of: %[1]s.%[2]s
def Get%[2]s(%[3]s):
`,
Copy link
Member

Choose a reason for hiding this comment

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

perhaps inject the documentation (if any) for the variable we get here?
so when we do help(mymodule.GetFoo) we get the documentation that was attached to the Foo variable on the Go side.

that would look like:

g.wrapper.Printf(`
# pythonization of: %[1]s.%[2]s
def Get%[2]s(%[3]s):
    """%[4]s""""
`,
  g.pkg.pkg.Name(),
  o.GoName(),
  strings.Join(funcArgs, ", "),
  o.Doc(),
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sbinet That's cool. I will do that :)

bind/gencffi_func.go Outdated
g.wrapper.Printf(`
# pythonization of: %[1]s.%[2]s
def Set%[2]s(%[3]s):
`,
Copy link
Member

Choose a reason for hiding this comment

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

same here.

@corona10 corona10 force-pushed the gsoc-cffi-step2 branch 2 times, most recently from 4cf7355 to 189b108 Compare June 13, 2017 14:10
@corona10
Copy link
Collaborator Author

@sbinet Done!

sbinet
sbinet reviewed Jun 13, 2017
View reviewed changes
main_test.go Outdated
testPkgWithCFFI(t, pkg{
path: "_examples/vars",
want: []byte(`Initial values
v1 = v1
Copy link
Member

Choose a reason for hiding this comment

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

perhaps also add here a test of the docstring?

see:
https://github.com/go-python/gopy/blob/master/_examples/empty/test.py#L10
https://github.com/go-python/gopy/blob/master/main_test.go#L399

Copy link
Member

Choose a reason for hiding this comment

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

(just for one of the vars, no need to do that for all of them. the mechanism is the same)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sbinet Just for the vars.__doc__? or for all the Get/Set functions?
Only vars__doc__ is okay but for each of Get/Set functions might be a problem.
The problem of Get/Set functions docstring is that output is different between Cpy2 and CFFI.

# CFFI output
doc(vars.GetV1()):
'returns vars.V1'

# CPy2 output
doc(vars.GetV1()):
''

Copy link
Member

Choose a reason for hiding this comment

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

and for one Get/Set pair.

it's OK to have a different reference for CFFI and for CPython-2.
(if there is no issue number for the CPython-2 backend to generate docstrings for Get/Sets of vars, I'll add one.)

@corona10
Copy link
Collaborator Author

Updated!

sbinet
sbinet approved these changes Jun 14, 2017
View reviewed changes
Copy link
Member

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

LGTM

one last thing: please squash the 2 commits together (keeping the commit message of the first commit), and I'll merge that PR.

thanks!

@corona10
bind, gencffi: Remove generating builders and support Vars and Consts.
2bad8a4 
- Remove the generating builder python script.
- Now, genCFFI interface is same as the genCpy.
- Support builtin-types Vars and Consts.
- Pass empty.go
- Pass cgo.go
- Pass vars.go
- Pass consts.go

Updates: go-python#102
@corona10
Copy link
Collaborator Author

@sbinet Thank you for reviewing this. :) PTAL

@sbinet sbinet merged commit 2bad8a4 into go-python:master Jun 14, 2017
@sbinet
Copy link
Member

sbinet commented Jun 14, 2017

thanks

@corona10 corona10 deleted the gsoc-cffi-step2 branch June 14, 2017 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbinet sbinet sbinet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@corona10 @sbinet