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.

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

Compiler #937

Merged
merged 102 commits into from
May 7, 2022
Merged

Compiler #937

merged 102 commits into from
May 7, 2022

Conversation

ephread
Copy link
Collaborator

@ephread ephread commented Feb 19, 2022 •
edited by smwhr
Loading

Here we go, last stretch before the compiler lands in inkjs.

Previous discussion in #931.

Remaining tasks

  • Remove ink-proof and port the relevant tests from upstream. → @ephread
    • Ensure the tests run against the distributable .js files. → @smwhr
  • Add new tests checking that the JavaScript compiler generates the same JSON as inklecate. → @ephread
  • Disable float detection (for now), until we have settled on a solution (see [Compiler] Limitations regarding JavaScript's number type #934). → @smwhr
  • Put the runtime and the compiler in different namespaces to match upstream and unify shared types (e. g. ErrorType)
  • Review the entire port.
  • Investigate whether running ink-proof in the CI makes sense.

The fate of inklecate can be tackled at a later stage, until we figure out how we want to distribute it (per #933). Let's focus on releasing the compiler for now.

Checklist

  • The new code additions passed the tests (npm test).
  • The linter ran and found no issues (npm run-script lint).

Feel free to suggest more tasks or discuss them. If you want to contribute, call dibs and create a PR against the compiler branch.

smwhr added 30 commits June 29, 2021 14:45
@smwhr
upgrade @rollup to allow node 16 to build
1679c49
@smwhr
bump actions to node 16
4faff1d
@smwhr
add workflow_dispatch
b3ddb07
@smwhr
compiler imported from furkle/ts-ink
31d0f57
@smwhr
some tests to verify the moving parts
61f5e6b
@smwhr
entrypoint script
84a4de3
@smwhr
simple story parsed successfully
c2fc5b5
@smwhr
fixing a missing callback
d824153
@smwhr
remove debugger
08c5cdc
@smwhr
fix FindAll and some nulls
3ae09cd
@smwhr
super function can be call only if defined as methods
a3ce047
@smwhr
remove debugger
92ebaf6
@smwhr
running from top to bottom : wrongly but surely !
43236d7
@smwhr
add Identifier.ts d213cda4ffa03a447b507e1d395ca3b80ea3b131
136c595
@smwhr
migrate to identifiers
1e86ce9
@smwhr
looking for the extraneous done
2ce987c
@smwhr
restore CheckForNamingCollisions + some nullification
fcb5a85
@smwhr
enforce some more nulls
b3aeac1
@smwhr
[WIP]
7c95ba2
@smwhr
compiling helloworld perfectly !
20ad53b
@smwhr
5/135 passing. Choices are not parsed correctly, maths neither
67cc8d4
@smwhr
merge upstream
c6be050
@smwhr
major error in parsedchoice
ab7af50
@smwhr
replaced all implicit casts to asOrNull
8879e65
@smwhr
ToString -> toString
b82a175
@smwhr
variable assignment ok
373666f
@smwhr
transfixes
46ea429
@smwhr
rename NumberExpression.ts
6852093
@smwhr
accept bool in NumberExpression
28f4211
@smwhr
transfixes on Path
c94c612
@ephread
Copy link
Collaborator Author

ephread commented Feb 22, 2022

@y-lohse, @smwhr #939 is ready for review, feel free to assign yourselves \o/

smwhr and others added 19 commits March 2, 2022 13:56
@smwhr
Merge pull request #941 from smwhr/remove-float-detection
de484d2 
Remove float detection
@smwhr
Merge remote-tracking branch 'y-lohse/compiler' into compiler-tests
f025ab3
@smwhr
allow redefinition of const if equals
8e93880
@smwhr
Fix : Variables › tests variable naming collision with flow
9bf98b3
@smwhr
Fix Knot > tests stitch naming collision
33ad96b
@smwhr
Fix Misc > tests return text warning
7bc7cae 
Fix Functions > tests function purity checks
@smwhr
missing content for : Choices > tests various blank choice warning
c5c7de2
@smwhr
fix typo in : Functions > tests argument name collision
e537629
@smwhr
case + correct message matched
ca931f9
@smwhr
linter
9fc4a6a
@smwhr
fix Optional
22808d6
@smwhr
lint
bd8a6e3
@smwhr
fix javascript tests
cd82c36
@ephread
feat: port the remaining tests from Test.cs and fix minor issues (#939)
398de79 
* feat: port the remaining tests from Test.cs and fix minor issues

* fix: typos in tests and remove it.only

* fix: broken test

* fix: default FileHandler
@smwhr
merge upstream
1b47a57
@smwhr
docs
7ab0373
@smwhr
comment on Number
f25e507
@smwhr
more docs
2d0605c
@smwhr
Merge pull request #944 from smwhr/compiler-tests
8db4e20
@smwhr smwhr requested a review from y-lohse May 1, 2022 13:21
@y-lohse
Copy link
Owner

y-lohse commented May 7, 2022

Thank you once again for all the work that has gone into this 😍
I reviewed the config files, the tests and the engine changes… the compiler is too much code or me to review at this point, but I trust you 😊. Well you and the unit test suite haha.

I don't see any blockers, just 2 questions for double checking:

  1. The different tasks in the first comment are all actually done I believe?
  2. This PR doesn't introduce breaking changes to any of the public classes?

If we're good on these 2 points we can go ahead and merge this and release the new version as far as I'm concerned 🥳

@smwhr
Copy link
Collaborator

smwhr commented May 7, 2022

  1. The different tasks in the first comment are all actually done I believe?
  • tests checking that the JavaScript compiler generates the same JSON as inklecate : not automated but ran manually
  • runtime and the compiler in different namespaces : TBD
  1. This PR doesn't introduce breaking changes to any of the public classes?

No changes at all to what was already exposed.

I'm pressing merge ! 🥳

@smwhr smwhr merged commit 61b5263 into master May 7, 2022
@y-lohse y-lohse deleted the compiler branch May 7, 2022 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y-lohse y-lohse Awaiting requested review from y-lohse

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

Successfully merging this pull request may close these issues.
3 participants
@ephread @y-lohse @smwhr