Skip to content

Create project command - #5

Merged
Fatme merged 1 commit into
masterfrom
fatme/create-project-command
Jul 14, 2014
Merged

Create project command#5
Fatme merged 1 commit into
masterfrom
fatme/create-project-command

Conversation

@Fatme

Copy link
Copy Markdown
Contributor

No description provided.

Comment threadlib/bootstrap.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just call it npm

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I tried it but the module that is in package.json is also named npm :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That shouldn't be a problem, I think. The package is resolved using require('npm') and our class is resolved using yok, so there shouldn't be a collision.

@Fatme

Copy link
Copy Markdown
ContributorAuthor

@tailsu I addressed all your comments. I suggest to extract common blocks in another PR. Agree?

@tailsu

Copy link
Copy Markdown
Contributor

Better do it now as we keep postponing these clean-ups and it slows us down in the long term.

@Fatme

Copy link
Copy Markdown
ContributorAuthor

@tailsu

1 similar comment
@Fatme

Copy link
Copy Markdown
ContributorAuthor

@tailsu

Comment threadlib/options.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use osenv.home() here

@tailsu

Copy link
Copy Markdown
Contributor

👍

Fatme added a commit that referenced this pull request Jul 14, 2014
@Fatme
Fatme merged commit 0646ab3 into masterJul 14, 2014
@Fatme
Fatme deleted the fatme/create-project-command branch July 14, 2014 06:28

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We are missing some %ses here.

@Fatme

Copy link
Copy Markdown
ContributorAuthor

goot catch :) I'll fix it.

Sign up for freeto 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

@Fatme@tailsu@ligaz