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

59 feature switch back to starklark standard #75

Merged
merged 7 commits into from
Nov 9, 2022

Conversation

hulto
Copy link
Collaborator

@hulto hulto commented Nov 3, 2022

What type of PR is this?

Add one of the following kinds:
/kind update

What this PR does / why we need it:

Updates the eldritch version from 0.6.0 to 0.9.0-pre which is still underdevelopment and will need to be version pinned once a new releases is cut.

Which issue(s) this PR fixes:

Fixes #59

@hulto hulto linked an issue Nov 3, 2022 that may be closed by this pull request
@hulto hulto closed this Nov 3, 2022
@hulto hulto reopened this Nov 3, 2022
@hulto
Copy link
Collaborator Author

hulto commented Nov 3, 2022

Can't use the official 0.8.0 release from cargo since it doesn't have the patch for rust object sizes changing.
image

@@ -4,7 +4,7 @@ version = "0.1.0"
edition = "2021"

[dependencies]
starlark = { git = "https://github.com/hulto/starlark-rust", branch = "v0.6.0-hulto" }
starlark = { git = "https://github.com/facebookexperimental/starlark-rust", rev = "7c6986a189c45591da9346159f5ddd50685bbf96" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pinned to a commit since no release is available for 0.9.0 yet.
0.9.0 is the only official version that contains the patches for rust struct size mismatch.

channel = "nightly-2022-11-03"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to use a more recent version since starlark uses "nightly".

#[display(fmt = "FileLibrary")]
pub struct FileLibrary();
starlark_simple_value!(FileLibrary);

impl<'v> StarlarkValue<'v> for FileLibrary {
starlark_type!("file_library");

fn get_methods(&self) -> Option<&'static Methods> {
fn get_methods() -> Option<&'static Methods> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Compiler error thrown when upgraded to 0.9.0 this seems to be the new pattern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

static RES: MethodsStatic = MethodsStatic::new();
RES.methods(methods)
}
}

impl Serialize for FileLibrary {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now required by starlark that all modules implement a serialize function.

where
S: Serializer,
{
serializer.serialize_none()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding a none serializer since we don't serialize our modules. ever.

@@ -48,58 +58,73 @@ impl<'v> UnpackValue<'v> for FileLibrary {
// This is where all of the "file.X" impl methods are bound
#[starlark_module]
fn methods(builder: &mut MethodsBuilder) {
fn append(_this: FileLibrary, path: String, content: String) -> NoneType {
fn append(this: FileLibrary, path: String, content: String) -> anyhow::Result<NoneType> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Starlark want's exactly anyhow::Result<T>
image

@@ -48,58 +58,73 @@ impl<'v> UnpackValue<'v> for FileLibrary {
// This is where all of the "file.X" impl methods are bound
#[starlark_module]
fn methods(builder: &mut MethodsBuilder) {
fn append(_this: FileLibrary, path: String, content: String) -> NoneType {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Starlark errors if not _this.
Tried using #[allow(dead_code)] above the function definition but the error persisted.

Receiver parameter must be named thisor have#[starlark(this)] annotation
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doing this:

    #[starlark(this)]
    fn append(_this: FileLibrary, path: String, content: String) -> anyhow::Result<NoneType> {

Throws the error.
image

#[derive(Copy, Clone, Debug, PartialEq, Display)]
use serde::{Serialize,Serializer};

#[derive(Copy, Clone, Debug, PartialEq, Display, ProvidesStaticType)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ProvidesStaticType
Is no a required implementation for modules.
Using the derive is an easy way to add it.

Copy link
Collaborator

@Cictrone Cictrone left a comment

Choose a reason for hiding this comment

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

All tests pass and seems legit!

@hulto hulto merged commit 5ee29a9 into main Nov 9, 2022
@hulto hulto deleted the 59-feature-switch-back-to-starklark-standard branch November 9, 2022 17:58
KCarretto pushed a commit that referenced this pull request Feb 1, 2024
 
59 feature switch back to starklark standard (#75)

Switched back to official starlark. Using 0.9.0 pre-release.
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.

[feature] Switch back to starklark standard
2 participants