search

gracelang / minigrace

Self-hosting compiler for the Grace programming language
39 stars 22 forks source link

Add master #299

Closed KimBruce closed 5 years ago

KimBruce commented 5 years ago

Please incorporate these changes to minigrace to enable type checker

apblack commented 5 years ago

I can't accept this pull-request as it stands, for a number of reasons:

  1. the tests don't pass. This may be because of an intentional change in behavior, but if so, the test needs to be changed to document this changed behaviour.

  2. The log messages don't correspond to the contents of the commit. For example, 8a2c876 and fc52fd5 have the same log message, and 74771f4 says that gct files are read using the parser and lexer, but they are not.

  3. The changes repeatedly modify the same things, documenting the real history of trying something and then backing it out. What We want in a PR is a clean commit history that shows only the changes that were successful. For a trivial example, 68fc61f and 93edf30 seem to be inverses of each other. The right thing to do in these situations is to squash the related changes together into a single commit, and write a new log message that describes the net change.

Since this is a relatively small set of commits, I'm going to try cleaning them up myself.

apblack commented 5 years ago

Although the commit message for 74771f4 says

gct files are now read using the parser and lexer

the commit does not make this change. These changes to xmodule are, I believe, essential for the type checker to work across modules. You should find them in and incorporate them into another PR.

apblack commented 5 years ago

I've just added three commits to the master branch — 127905d, f4d7e7c, and 0e7371a. I believe that these incorporate the changes from the above pull request. As noted above, these commits do not incorporate the changes that use the parser and the toGrace methods on the ast to build the gct string, because these changes are not in this PR.