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

Install with no arguments #576

Merged
merged 3 commits into from
Feb 4, 2017
Merged

Install with no arguments #576

merged 3 commits into from
Feb 4, 2017

Conversation

gigabo
Copy link
Contributor

@gigabo gigabo commented Feb 3, 2017

Use temporary package.json files.

This is groundwork for:

  • Pluggable npm clients (yarn).
  • A lerna upgrade command.

This is a port of asini/asini#45. Please see original PR commentary during review.

Use temporary package.json files.

This is groundwork for:

- Pluggable npm clients (yarn).
- A `lerna upgrade` command.

This is a port of asini/asini#45.
// Return a tuple like ["foo", "^1.0.0"].
// Handles scoped packages.
// Returns undefined for version if none specified.
static splitVersion(dep) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I share @rygine's enthusiasm for regexes, but it seems that you could write tests for splitVersion directly that verified that it works as intended, which would make me feel much better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Added some tests.

@@ -39,6 +39,16 @@ export default class FileSystemUtilities {
fs.writeFile(filePath, ensureEndsWithNewLine(fileContents), callback);
}

@logger.logifyAsync()
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like most FileSystemUtilities have tests; seems worthwhile to add some for these as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another good call. Added some more tests.

@doug-wade doug-wade merged commit 3e7637e into lerna:master Feb 4, 2017
@lock
Copy link

lock bot commented Dec 27, 2018

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants