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

Adding load(fname) and save(fname) - save being really eval_file(fname) #122

Merged
merged 7 commits into from
Aug 11, 2024

Conversation

ldemailly
Copy link
Member

@ldemailly ldemailly commented Aug 11, 2024

Fixes #38
Fixes #125

tests have been manual, loading all the examples, saving, reloading comparing (how I stepped on #125)
not exactly sure how to ... actually test this

constants if not exact might cause an issue

@ldemailly ldemailly requested a review from ccoVeille August 11, 2024 21:57
@ldemailly
Copy link
Member Author

because I moved some code from one file (eval.go) to another (eval_api.go) it's probably easier to review individual commits

@@ -158,3 +173,42 @@ func evalFunc(env any, name string, args []object.Object) object.Object {
}
return res
}

func saveFunc(env any, _ string, args []object.Object) object.Object {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this method must comply with an interface?

You should add a comment then, because the unused string is awkward

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah it's defined a bit above

	jsonFn.Callback = saveFunc // save to file.

which is

// ExtFunction is the signature of what grol will call when the extension is invoked.
// Incoming arguments are validated for type and number of arguments based on [Extension].
// eval is the opaque state passed from the interpreter, it can be used with eval.Eval etc
// name is the function name as registered under.
type ExtFunction func(eval any, name string, args []Object) Object

like everything in the file, in short it doesn't need to know its own name

Comment on lines +109 to +110
n := 0
for _, k := range keys {
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something but either you add a comment explaining why you need n

or you do that

Suggested change
n := 0
for _, k := range keys {
for n, k := range keys {

Then return n+1 at the end of the function

Copy link
Member Author

Choose a reason for hiding this comment

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

n would be out of scope...

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do var k string though and drop the := though it seems icky to use loop vars outside of the loop

Copy link
Member Author

Choose a reason for hiding this comment

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

but that'd be incorrect for empty keys

Copy link
Member Author

Choose a reason for hiding this comment

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

so in short n counts the number of succcesful writes, it happens to be len(keys) if there are no errors but can be shorter if there is an error.

@ldemailly ldemailly merged commit 4919d5a into main Aug 11, 2024
1 check passed
@ldemailly ldemailly deleted the load_save branch August 11, 2024 23:36
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.

can't parse large numbers add save/load
2 participants