Skip to content

Better importing (caching & public API) #318

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

Merged
merged 1 commit into from
Sep 23, 2019

Conversation

sbarzowski
Copy link
Collaborator

  • It adds new API which allows using VM for importing
    files in external tools "just as Jsonnet would". This is
    primarily intended for use in static analysis tools.
  • Imports are now cached between evaluate calls. This may improve
    performance significantly for some users. I would like to add
    some way of achieving this with commandline in the future.
  • Additional layer of caching was internally added - AST level.
    This was necessary so that Jsonnet could always return the same
    exact AST when asked multiple times (meaning the same pointers).

@sbarzowski
Copy link
Collaborator Author

It was a side-effect of the work on the Linter which needs to go through imports to be really useful.

@coveralls
Copy link

coveralls commented Sep 9, 2019

Coverage Status

Coverage decreased (-0.1%) to 77.529% when pulling 7fb5d80 on sbarzowski:better-imports into fbde25b on google:master.

imports.go Outdated
@@ -97,6 +110,21 @@ func (cache *importCache) importData(importedFrom, importedPath string) (content
return
}

func (cache *importCache) importAST(importedFrom, importedPath string) (ast.Node, string, error) {
// TODO(sbarzowski) perhaps we can cache the AST, so that we go through parsing twice
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment doesn't make sense

@sparkprime
Copy link
Collaborator

Seems reasonable to expose this.

* It adds new API which allows using VM for importing
  files in external tools "just as Jsonnet would". This is
  primarily intended for use in static analysis tools.
* Imports are now cached between evaluate calls. This may improve
  performance significantly for some users. I would like to add
  some way of achieving this with commandline in the future.
* Additional layer of caching was internally added - AST level.
  This was necessary so that Jsonnet could always return the same
  exact AST when asked multiple times (meaning the same pointers).
@sbarzowski
Copy link
Collaborator Author

I fixed that obsolete comment, another typo, and a bug (vm.flushValueCache was empty before).

@sparkprime Do you want to take another look at it?

@sparkprime
Copy link
Collaborator

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants