The Wayback Machine - https://web.archive.org/web/20201106133142/https://github.com/d5/tengo/pull/126
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

Decoupled stdlib from vm, script and compiler #126

Merged
merged 7 commits into from Mar 4, 2019
Merged

Conversation

@geseq
Copy link
Collaborator

@geseq geseq commented Mar 2, 2019

stdlib is fully optional. this is in prep for #68

runVMError(t, input, nil, userModules, nil, expected)
}

func expectErrorWithBuiltinModules(t *testing.T, input string, builtinModules map[string]*objects.Object, expected string) {

This comment has been minimized.

@golangcibot

golangcibot Mar 2, 2019

expectErrorWithBuiltinModules is unused (from deadcode)

runVMError(t, input, nil, nil, builtinModules, expected)
}

func expectErrorWithUserAndBuiltinModules(t *testing.T, input string, userModules map[string]string, builtinModules map[string]*objects.Object, expected string) {

This comment has been minimized.

@golangcibot

golangcibot Mar 2, 2019

expectErrorWithUserAndBuiltinModules is unused (from deadcode)

@@ -171,8 +187,15 @@ func traceCompileRun(file *ast.File, symbols map[string]objects.Object, userModu
symTable.DefineBuiltin(idx, fn.Name)
}

bm := make(map[string]bool)
if builtinModules != nil {

This comment has been minimized.

@golangcibot

golangcibot Mar 2, 2019

S1031: unnecessary nil check around range (from gosimple)

@@ -171,8 +187,15 @@ func traceCompileRun(file *ast.File, symbols map[string]objects.Object, userModu
symTable.DefineBuiltin(idx, fn.Name)
}

bm := make(map[string]bool)
if builtinModules != nil {
for k, _ := range builtinModules {

This comment has been minimized.

@golangcibot

golangcibot Mar 2, 2019

S1005: should omit value from range; this loop is equivalent to for k := range ... (from gosimple)

@d5
Copy link
Owner

@d5 d5 commented Mar 2, 2019

Hey, nice work as always. I have 2 comments:

  1. I think we don't have to remove stdlib from tests. Test packages will not be included when Tengo is imported by other packages.
  2. What's your plan for cmd/tengo? We will have to ship 2 different binaries? One with stdlib, another without stdlib?
@geseq
Copy link
Collaborator Author

@geseq geseq commented Mar 2, 2019

  1. I removed it from tests and moved some to stdlib (tests for os) not because of importing but for improved separation of concerns. The tests in vm should ideally be testing the vm itself and not the stdlib and the same goes for scripts. Stdlib now has some of the tests that were originally in the vm that test the stdlib functionality. I think stdlib might even do with some more tests but that’s a separate issue altogether.
  2. Yes, IMO binaries would probably be the most straightforward way to deal with this.
@geseq
Copy link
Collaborator Author

@geseq geseq commented Mar 2, 2019

Also, I’ll try to work on improving the tests in the packages at some point. There’s some redundant tests that I think can be cleaned up and the focus narrowed a bit more tightly.

@d5
Copy link
Owner

@d5 d5 commented Mar 2, 2019

Ok. Makes sense. We will have to update documentations, cmd/tengo (likely just another package to include all stdlib?), and, also the build scripts accordingly.

Repository owner deleted a comment from geseq Mar 3, 2019
geseq added 2 commits Mar 4, 2019
var constants []objects.Object

for {
_, _ = fmt.Fprintf(out, replPrompt)

This comment has been minimized.

@golangcibot

golangcibot Mar 4, 2019

SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (from staticcheck)

geseq added 2 commits Mar 4, 2019
@geseq
Copy link
Collaborator Author

@geseq geseq commented Mar 4, 2019

Tengo binary is quite small. I'm not entirely sure the second binary without stdlib bundled is worth it. A flag in the main binary to disable stdlib might be enough.

@d5 d5 merged commit c437def into d5:master Mar 4, 2019
2 checks passed
2 checks passed
GolangCI No issues found!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geseq geseq deleted the geseq:stdlibdecouple branch Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.