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

Ember fixes #115

Merged
merged 5 commits into from
Feb 1, 2017
Merged

Ember fixes #115

merged 5 commits into from
Feb 1, 2017

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Jan 31, 2017

Fixes #111
Fixes #112
Fixes #114
Fixes #108

Have you read the section in CONTRIBUTING.md about pull requests?

Yes

Are your changes appropriately documented?

Yes

Do your changes have sufficient test coverage?

N/A

Does the testsuite pass successfully on your local machine?

Yes

Summarize your changes:

Fix the things 😆
Pretty straight forward

  • Wrap all console logs in interactive checks
  • Remove process.exit calls 'cos that's just plain wrong
  • Provide a config option to the import API for updateScripts
  • Default electronVersion to @latest

malept
malept previously requested changes Jan 31, 2017
@@ -205,10 +206,11 @@ export default async (providedOptions = {}) => {
}), null, 2));
});

console.info('NOTE: You might be able to remove your `.compilerc` file completely if you are only using the `es2015` and `react` presets'.yellow);
if (interactive) console.info('NOTE: You might be able to remove your `.compilerc` file completely if you are only using the `es2015` and `react` presets'.yellow); // eslint-disable-line max-len
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can do a similar thing to Packager here:

function info (interactive, message) {
  if (interactive) {
    console.info(message)
  }
}

Same with warn.

src/api/make.js Outdated
@@ -49,19 +49,18 @@ export default async (providedOptions = {}) => {
});

if (platform && platform !== process.platform && !(process.platform === 'darwin' && platform === 'mas')) {
console.error('You can not "make" for a platform other than your systems platform'.red);
process.exit(1);
throw 'You can not "make" for a platform other than your systems platform';
Copy link
Member

Choose a reason for hiding this comment

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

throw "You cannot run 'make' for a platform other than your system's platform";

d('installing devDependencies');
await installDepList(dir, devDeps, true);
d('installing electron-prebuilt-compile');
await installDepList(dir, [`electron-prebuilt-compile@${electronVersion || 'latest'}`], false, true);
Copy link
Member

Choose a reason for hiding this comment

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

I had to check for myself what happened when you ran npm install --save --exact electron-prebuilt-compile@latest but it does what we want 👍

@malept
Copy link
Member

malept commented Jan 31, 2017

Also it would be kind of nice if there was a test for the updateScripts change.

@MarshallOfSound
Copy link
Member Author

@malept We don't have any import tests yet (need to come up with a templatey bootstrapping thing to test it)

@MarshallOfSound
Copy link
Member Author

@malept Travis CI is currently under ridiculous load. If you're OK with the PR we can merge it.

I've run the test suite locally on macOS

@anulman
Copy link
Contributor

anulman commented Feb 1, 2017

❤️ you all. Thanks @MarshallOfSound @malept 😄

@malept malept merged commit f223df8 into master Feb 1, 2017
@malept malept deleted the ember-fixes branch February 1, 2017 18:09
@malept
Copy link
Member

malept commented Feb 1, 2017

@MarshallOfSound feel free to release a new version, I don't think I'll have time to do that today.

dsanders11 pushed a commit that referenced this pull request Jan 14, 2023
chore: Updates for everyone
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.

3 participants