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

share definitions of constraints, types, and functions #514

Merged
merged 1 commit into from
Apr 1, 2018

Conversation

davidchambers
Copy link
Member

Currently, each Sanctuary function is defined once for each Sanctuary module created. This pull request restructures the code so that constraints, types, and functions are defined just once.

@davidchambers davidchambers requested a review from Avaq March 24, 2018 23:06
Sum.Type
])
});
}(require('.')));
Copy link
Member Author

Choose a reason for hiding this comment

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

We define S here for the benefit of the doctests.

// createSanctuary :: Options -> Module
function createSanctuary(opts) {

/* eslint-disable indent */
Copy link
Member Author

Choose a reason for hiding this comment

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

This pull request solves the problem discussed in #457 and #467.

/cc @futpib

index.js Outdated
createSanctuary);
function create(opts) {
var S = {
env: defaultEnv,
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be opts.env rather than defaultEnv, should it not? If others agree, I'll open a separate pull request for this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

S[name] = def(name, _[name].consts, _[name].types, _[name].impl);
});
return S;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This function is several thousand lines smaller than createSanctuary. ;)

@davidchambers davidchambers force-pushed the davidchambers/create branch 3 times, most recently from 8cd7de9 to e4e60dd Compare March 28, 2018 11:24
@gabejohnson
Copy link
Member

To make certain I understand, you pulled the definitions out of createSanctuary so they're defined on _ which itself is defined in the closure and referenced in create. Is this correct?

@davidchambers
Copy link
Member Author

Before:

function createSanctuary(opts) {
  var S = {};
  //  Sanctuary functions (including ‘create’) defined here.
  return S;
}

return createSanctuary({checkTypes: true, env: defaultEnv});

require('sanctuary').create(...) would result in each exported function being defined twice.

After:

//  Sanctuary functions (including ‘create’) defined at the top level.

return create({checkTypes: true, env: defaultEnv});

require('sanctuary').create(...) would now result in each exported function being defined just once, since create now reuses the functions defined at the top level (stored in the _ string map).

@gabejohnson
Copy link
Member

LGTM

//. . ({x, y, width, height})
//. . )
//. createRect
Copy link
Member Author

Choose a reason for hiding this comment

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

🎉

@davidchambers davidchambers force-pushed the davidchambers/create branch from 3d435a4 to 565195f Compare April 1, 2018 20:12
@davidchambers davidchambers merged commit aebbf9c into master Apr 1, 2018
@davidchambers davidchambers deleted the davidchambers/create branch April 1, 2018 20:18
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.

2 participants