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

Support implementing functions in Python #232

Merged
merged 5 commits into from
Aug 14, 2024
Merged

Support implementing functions in Python #232

merged 5 commits into from
Aug 14, 2024

Conversation

brandonchinn178
Copy link
Contributor

Resolves #230

@AlmogBaku
Copy link

lgtm @jordemort

@jordemort
Copy link
Contributor

Thanks, this looks pretty good. Two things:

  • The build fails on Python 3.7, 3.8, 3.9 because the exception message is not exactly what the test expects. I need to update my Python versions here and drop 3.7 because it's EOL, but 3.8 and 3.9 are still supported, so please loosen up the test so that it works there.
  • I'm not sure exactly where this capability should be mentioned in the docs, but it should be mentioned somewhere. Unfortunately, the bulk of the documentation is embedded in docstrings in starlark.c in a very ugly way.

@brandonchinn178
Copy link
Contributor Author

@jordemort How's that?

@jordemort
Copy link
Contributor

This is looking good; the macOS builds are broken through no fault of yours. I fixed that, and dropped Python 3.7, and promoted Python 3.12 to production, and updated a few other dependencies. Would you mind rebasing on to / merging in my latest main? After that, if all the tests are passing, I think I am ready to merge it.

@brandonchinn178
Copy link
Contributor Author

@jordemort Should be good to go!

@brandonchinn178
Copy link
Contributor Author

@jordemort any updates?

@jordemort jordemort merged commit a7a5317 into caketop:main Aug 14, 2024
44 of 45 checks passed
@jordemort
Copy link
Contributor

@brandonchinn178 I've merged it! There's a few other things I want to get to before doing another release, though, and I'm not sure when I'm going to get to them; if you or anyone else wants to run with these before I am able to make time, I am happy to review and merge the results:

  • It looks like I need to enable 2FA on test.pypi before it will let me do test uploads again
  • The embedded version of Starlark needs to be updated, but it's going to necessitate an API break, re: Upgrade starlark-go (20240311180835-efac67204ba7) #204
  • There's some open issues related to adding support for structs, json functions, and other loadable stuff that I would like to include in a 2.0, if I'm going to have to break the API anyway

@brandonchinn178
Copy link
Contributor Author

Thanks @jordemort! Is there a reason why starlark-go "has" to be updated? I don't see why function support can't be released now in the 1.x series, decoupled from implementing all those 2.x features.

The only reason I'm asking is because it'd be nice to have a prebuilt wheel with function support, so I don't have to require users of my script to have golang installed to install python-starlark-go

@brandonchinn178 brandonchinn178 deleted the bchinn-functions branch August 14, 2024 16:40
@jordemort
Copy link
Contributor

@brandonchinn178 I'll see if I can get an interim release out for you, I just feel bad pushing one with such an out-of-date release of starlark-go. Hopefully I'll get some time later in the week to chase the bats out of the CI.

@brandonchinn178
Copy link
Contributor Author

I'd really appreciate that! We don't mind the old version of starlark-go. Thanks a ton!

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.

Support defining functions in Python
3 participants