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

[student-libraries] Expose functions, comments, and parameters from the JSInterpreter #30853

Merged
merged 5 commits into from Sep 27, 2019

Conversation

jmkulwik
Copy link
Contributor

@jmkulwik jmkulwik commented Sep 19, 2019

Added functionality to the JSInterpreter to parse student code for functions along with their comments and parameters.

For a sample of what student comments could look like, see this project: https://studio.code.org/projects/applab/TUd7VzCGUL5pVK-LqRLi-9Z1xtw-R-KinEDnVeA2pag/edit

This pairs with this PR in the JSInterpreter library: code-dot-org/JS-Interpreter#32

This is in preparation for the student-libraries feature described in this spec: https://docs.google.com/document/d/1VxMmdHdIeBWCXKofkjEy9MuWcrzeEW3Zt-Ir8pINYdA/edit

@jmkulwik jmkulwik changed the title Prod libraries s3 [student-libraries] Expose functions, comments, and parameters from the JSInterpreter Sep 19, 2019

let comments = [];
let options = {
onComment: (isBlockComment, text, startLocation, endLocation) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How/when is this option used? Is it https://github.com/code-dot-org/JS-Interpreter/blob/master/acorn.js#L81? Might be helpful to include a comment explaining since it's not immediately obvious what this is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 PTAL

@@ -245,6 +246,78 @@ export default class JSInterpreter {
}
}

/**
* Builds a list of objects that contain all metadata about any functions in
* the gived code string. Each object in the returned list has the following
Copy link

Choose a reason for hiding this comment

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

nit: typo on given

@@ -245,6 +246,78 @@ export default class JSInterpreter {
}
}

/**
* Builds a list of objects that contain all metadata about any functions in
* the gived code string. Each object in the returned list has the following
Copy link

Choose a reason for hiding this comment

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

Can we use the @return tag to be consistent with our commenting style and include the expected type for the returned object?

@@ -7,6 +7,79 @@ import JSInterpreter from '@cdo/apps/lib/tools/jsinterpreter/JSInterpreter';
describe('The JSInterpreter class', function() {
var jsInterpreter;

describe('static function getFunctionsAndMetadata', () => {
Copy link

Choose a reason for hiding this comment

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

This test coverage is great!
chefkiss

@jmkulwik jmkulwik merged commit 19948ef into staging Sep 27, 2019
@jmkulwik jmkulwik deleted the prod-libraries-s3 branch September 27, 2019 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants