Skip to content

Add Chapter 4 #23

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 29 commits into from
Oct 29, 2018
Merged

Add Chapter 4 #23

merged 29 commits into from
Oct 29, 2018

Conversation

remo5000
Copy link
Contributor

@remo5000 remo5000 commented Aug 1, 2018 •
edited
Loading

Features

  • Added some functions from the docs
  • Imported some type checking functions from a previous repository
  • Used external parse function from sourceacademy

TODO

  • Add a parse function
  • Add the source for the parse function

Changing the grammar of the metacircular parser will be raised as an issue (#24 )

remo5000 added 2 commits August 1, 2018 13:21
@remo5000
Add specified function definitions
bcf7d2e 
Misc functions are to be imported after this
@remo5000
Import type checking and error methods
dc8ebe1 
Only some are used, but all of them are imported to keep this file up to
date with all the functions that were present before
@coveralls
Copy link

coveralls commented Aug 1, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 210

  • 37 of 514 (7.2%) changed or added relevant lines in 3 files are covered.
  • 59 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-5.4%) to 42.788%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/createContext.ts 12 20 60.0%
src/stdlib/misc.ts 16 24 66.67%
src/stdlib/parser.js 9 470 1.91%
Files with Coverage Reduction New Missed Lines %
src/interop.ts 1 65.06%
src/stdlib/misc.ts 1 62.12%
src/createContext.ts 2 83.46%
src/interpreter-errors.ts 2 59.76%
src/interpreter.ts 53 68.09%
Totals Coverage Status
Change from base Build 21: -5.4%
Covered Lines: 835
Relevant Lines: 1743
💛 - Coveralls

@ning-y ning-y added the _work in progress Issue currently being worked on; discussion should name PR label Aug 1, 2018
@remo5000 remo5000 removed the _work in progress Issue currently being worked on; discussion should name PR label Aug 2, 2018
@ning-y
Copy link
Member

ning-y commented Aug 7, 2018

@remo5000 You gotta fix the tests

@remo5000
Fix tests not compiling
10e1fa9 
There was a problem with transpiling the files when testing, because of
the .js files used. I first added a transformer for the .js files (that
didnt have one at first, throwing an error at the `export` line in
parser.js). Then, I added the presets required, which were displayed as
missing presets when trying to test
@remo5000
Copy link
Contributor Author

remo5000 commented Aug 8, 2018 •
edited
Loading

Tried a fix, let's see if it works for Travis

@ning-y
Copy link
Member

ning-y commented Aug 8, 2018

You've tested the the js files from this new setting in 10e1fa9 works on frontend?

@remo5000
Update build to a script
df55ab5 
This script will run the typescript build, and then copy over the
required .js file over. Note that to add more .js files, the build
script will need to be modified.
@remo5000
Copy link
Contributor Author

remo5000 commented Aug 9, 2018

Had a problem with the .js files not being copied. Please review the documentation, thanks! The module works with the frontend now

ning-y
ning-y previously requested changes Aug 10, 2018
View reviewed changes
scripts/build.sh Outdated
@@ -0,0 +1,7 @@
# Compile all .ts files
tsc
Copy link
Member

Choose a reason for hiding this comment

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

Do a && so cp does not run if tsc fails.

@ning-y ning-y mentioned this pull request Aug 14, 2018
Merged
@ning-y
Copy link
Member

ning-y commented Aug 14, 2018

Bump

@remo5000
Copy link
Contributor Author

Missed the notif, done! @ningyuansg

@ning-y ning-y requested a review from thenaesh August 20, 2018 13:31
martin-henz
martin-henz requested changes Aug 28, 2018
View reviewed changes
Copy link
Member

@martin-henz martin-henz left a comment

Choose a reason for hiding this comment

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

This is not documented in docs/source_4, which still has JSON.stringify

Two places: beginning under changes, and "Interpreter Support"

@martin-henz
Copy link
Member

I think some more work is needed for this. The new specs for Source §4 are here.
source_4.pdf

According to Yuchuan, apply_in_underlying_javascript is missing from the implementation, see page 7 of specs.

@martin-henz martin-henz dismissed ning-y’s stale review October 29, 2018 09:42

A bit of a rush; no time to look at this

@martin-henz martin-henz merged commit d7b7ed0 into master Oct 29, 2018
@thomastanck thomastanck mentioned this pull request Nov 16, 2018
Closed
@thomastanck thomastanck deleted the chapter-four branch December 9, 2018 06:32
Anonymxtrix added a commit that referenced this pull request Mar 23, 2021
@Anonymxtrix
Fix bug highlighted in modules issue #23
578ff5e
martin-henz pushed a commit that referenced this pull request Mar 24, 2021
@Anonymxtrix
[Modules]: Bugfixes to modules issues 22, 23 and 28 (#916)
474db1f 
* Add error for undefined empty string imports

* Add error trace for module related errors in Source and refactored moduleLoader.ts

* Fix bug highlighted in modules issue #23

* Fix bug highlighted in modules issue #28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-henz martin-henz martin-henz approved these changes

@ning-y ning-y ning-y left review comments

@thenaesh thenaesh Awaiting requested review from thenaesh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@remo5000 @coveralls @ning-y @martin-henz