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

feat: add solidity-style sources compilation #5

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

d1ll0n
Copy link
Collaborator

@d1ll0n d1ll0n commented Sep 23, 2019

Description

Added ability to provide a sources object in place of a path in the filesystem.
Sources object has file names as keys and file data as values.

Changes

src/parser.js

  • added function for parsing from sources object
  • changed exported functions to determine whether to use sources or file parsing function

src/parser.spec.js

  • added parser tests for sources object

src/runtime.js

  • added the ability to pass sources object with runtime

src/runtime.spec.js

  • added runtime tests using sources object

Sorry for the messy commits.

@d1ll0n d1ll0n added the enhancement New feature or request label Sep 25, 2019
@d1ll0n d1ll0n requested a review from ArnSch September 26, 2019 06:38
Copy link
Contributor

@ArnSch ArnSch left a comment

Choose a reason for hiding this comment

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

Comments on the main getSourcesFileContents fn, which I think can be made cleaner.

Also, you should create branches off of this repo now that you're a collaborator :)

const parentPathOf = (str) => (m = /(.*)\/(?!\/)(.*)\.huff/g.exec(str)) && m[1];
const recurse = (filename, parentPath) => {
const fileName = normalizePath(filename)
const relativePath = path.join(parentPath || '', fileName);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can make the interface to this method a tiny bit clearer by hoisting this || to instead be a default parameter for parentPath

@@ -734,20 +734,86 @@ parser.getFileContents = (originalFilename, partialPath) => {
return { filedata, raw };
};

const normalizePath = (str) => path.join('', str);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is far from a clean codebase, but think we can help it out by putting some of these smaller helper functions into their own namespace.

let imported = [];
let test = formatted.match(grammar.topLevel.IMPORT);
while (test !== null) {
const importStatement = formatted.match(grammar.topLevel.IMPORT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Your test variable will always be up to date, and you don't need to recompute the regex match.

while (test !== null) {
const importStatement = formatted.match(grammar.topLevel.IMPORT);
const empty = ' '.repeat(importStatement[0].length);
formatted = empty + formatted.slice(importStatement[0].length);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make the recurse function return the parsed file data, with import statements replaced with the data of the underlying file they refer to, you can replace empty with the return of recurse(importedFilename, parentPathOf(relativePath)), getting rid of quite a few ... and a reduce.

i.e. have your recurse aggregate all imports into one source string, and return that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants