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

Add templating and template ASTPass.swift #408

Merged
merged 1 commit into from Oct 11, 2018

Conversation

nvgrw
Copy link
Member

@nvgrw nvgrw commented Oct 11, 2018

Implementation Overview

Adds templating to ASTPass.swift.
We refactored some of the calls to process and postProcess as their labels were inconsistently named.

Templates have the extension .template.swift rather than .swift.
Generated sources are not committed to source control and rather are generated as part of the generate target (a requirement of all, release, test) to the .derived-sources directory.

Adds further node modules as dependencies. We already require node to run truffle so we think the impact of this is minimal. Installation is simplified by a top-level module in the root of the repository, so npm install should be run prior to compilation. We decided not to invoke NPM on every build because the installation of dependencies is a one-off process. Opinions on this decision are welcome.

Adding new templates requires changes to a few parts of the build setup:

  • add an exclusion to Package.swift so that the project does not compile the .template.swift file. We could have chosen a different file extension but these files are still inherently Swift and Xcode highlights any file with the .swift extension automatically. Unfortunately exclusions don't support globs, or we would've excluded any file ending in .template.swift.
  • add an inclusion to the .derived-sources directory to Package.swift if templating a file outside of the AST module.
  • make sure to disable (and later reenable) the linter for the template file. Swiftlint does not like nunjucks/jinja and globbing exclusions is again not possible.

We chose nunjucks as other Swift-first templating solutions don't work well on Linux, and nunjucks over jinja as we wanted to avoid adding Python as a(nother) dependency.

Notes

  • reformat ASTPass.swift to single lines
  • add test jinja2 template
  • add .derived-sources to gitignore and update Package.swift to include derived source
  • added codegen using jinja2
  • turn codegen into a node package and add to top level module
  • add generate target to makefile
  • pre-refactor extraction
  • rename enumCase => enumMember
  • rename literalToken => token
  • fix travis yaml

Implemented together with @Aurel300

@nvgrw
Copy link
Member Author

nvgrw commented Oct 11, 2018

The CI crashed running apt-get. I'll try re-running it.

@nvgrw
Copy link
Member Author

nvgrw commented Oct 11, 2018

I don't have the ability to re-run this. If someone else could restart it that would be great.

* reformat ASTPass.swift to single lines

* add test jinja2 template

* add .derived-sources to gitignore and update Package.swift to include derived source

* added codegen using jinja2

* turn codegen into a node package and add to top level module

* add generate target to makefile

* pre-refactor extraction

* rename enumCase => enumMember

* rename literalToken => token

* fix travis yaml
@SusanEisenbach
Copy link
Member

SusanEisenbach commented Oct 11, 2018 via email

@Aurel300
Copy link
Contributor

@SusanEisenbach The CI was restarted by Nik with a push. Only a review is now needed to merge this.

Copy link
Member

@DJRHails DJRHails left a comment

Choose a reason for hiding this comment

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

Looks good and I think all the decisions you've mentioned are very sensible.

@DJRHails DJRHails merged commit 765b401 into flintlang:master Oct 11, 2018
@nvgrw nvgrw deleted the outbound/templating branch October 11, 2018 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants