-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
@@ -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" } |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static RES: MethodsStatic = MethodsStatic::new(); | ||
RES.methods(methods) | ||
} | ||
} | ||
|
||
impl Serialize for FileLibrary { |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[derive(Copy, Clone, Debug, PartialEq, Display)] | ||
use serde::{Serialize,Serializer}; | ||
|
||
#[derive(Copy, Clone, Debug, PartialEq, Display, ProvidesStaticType)] |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
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
to0.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