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

Cli should use webgme-engine as a dependency #215

Merged
merged 13 commits into from Oct 9, 2017

Conversation

pmeijer
Copy link

@pmeijer pmeijer commented Oct 5, 2017

No description provided.

Copy link
Contributor

@brollb brollb 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 overall! Just a little change in the test that I suspect will cause it to break in the future

// Compare the package.json values
var packageJSON = path.join(initProject, 'package.json'),
toolJson = path.join(__dirname, '..', 'package.json'),
toolDeps = require(toolJson).dependencies,
deps = require(packageJSON).peerDependencies;

assert.equal(deps.webgme, toolDeps.webgme);
assert.equal(deps.webgme, toolDeps['webgme-engine']);
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't currently checking the major version... I think this test will fail after a patch/minor release of webgme (without an update to webgme-engine), right?

Copy link
Author

Choose a reason for hiding this comment

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

👍

Copy link
Author

Choose a reason for hiding this comment

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

Actually no. It's not reading the installed version but rather the required version which is ^2.0.0.

@brollb brollb merged commit fa7bb87 into master Oct 9, 2017
@brollb brollb deleted the webgme-engine-as-dependency branch October 9, 2017 22:50
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

2 participants