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

adds 'probot scaffold' command; closes #98 #139

Closed
wants to merge 1 commit into from

Conversation

boneskull
Copy link
Contributor

@boneskull boneskull commented May 5, 2017

Here's basic support for scaffolding a plugin.

In the end, I didn't go with init for the command name. To me, init means "let's start a Probot-based project", not necessarily "let's create a new Probot plugin".

EDIT Renamed to probot init

Anyway, here's a screencast of what it does.

As you can see from the output, it wants this PR to fix the plugin template to land.

Please let me know if you have questions about how I'm guessing default values (see user.js).

Copy link
Contributor

@bkeepers bkeepers left a comment

Choose a reason for hiding this comment

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

🎉 this is awesome, thanks for getting it started! We'll want to update the plugin docs before this lands, but let's talk about some of the details first.

Overall, the guessing of values is nice, but I'm not convinced it's worth all the extra dependencies. It makes me wonder if this command should be extracted to a separate package, and keep probot as just the runtime components. What do you think?

In the end, I didn't go with init for the command name. To me, init means "let's start a Probot-based project", not necessarily "let's create a new Probot plugin".

I get what you're saying, but I don't love scaffold. I'd prefer init or maybe even new.

'use strict';

const path = require('path');
const superb = require('superb');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is amusing, but I don't think it's worth the dependency.

default: `A ${superb()} Probot plugin`,
message: 'Description of plugin:',
when: !program.description
},
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 showing up in the prompt, and the generated description is blank.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that's because commander has absconded with program.description. program.name, which I had tried to use for something else, is a function. This is all very strange. I'm not a big fan of commander; there's a bit too much magic for me.

Re superb: Yeoman uses it, so I wanted to conform.

type: 'input',
name: 'email',
default() {
return guessEmail();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is returning undefined for me:

? Plugin author's full name: Brandon Keepers
? Plugin author's email address: undefined

I have a git email configured:

$ git config user.email
bkeepers@github.com

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • You must be running this command from a working copy. Is your email address set in that directory's .git/config?
  • What is the output of git config --global user.email?

Not sure if this is my fault or a git-user-email bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

You must be running this command from a working copy.

Ah, yep, I was.

Is your email address set in that directory's .git/config?

Nope

What is the output of git config --global user.email?

git config --global user.email
bkeepers@github.com

@boneskull
Copy link
Contributor Author

boneskull commented May 5, 2017

Overall, the guessing of values is nice, but I'm not convinced it's worth all the extra dependencies.
It makes me wonder if this command should be extracted to a separate package, and keep probot as just the runtime components. What do you think?

I'm not sure how easy this would be with commander, though I can check it out. At minimum, I'm pretty sure bin/probot-init.js would need to exist as a facade for the module.

But...

  • I feel like the executable (bin/probot-init.js) should stay mostly as-is (with your suggested rename).
  • lib/user.js could be split out into its own published module. It's an abstraction around discovering Git & GitHub user info, which is useful.
  • It might be worthwhile to extract the prompts themselves into a separate module, since these are questions that many scaffolding tools ask or need to know. But most of those tools are Yeoman generators!
  • Unless Probot wants to go the "monorepo" route (which has its drawbacks), splitting bin/probot-init.js into its own standalone module will incur an added maintenance overhead.

What I think I'd like to do is address the problem you're seeing in lib/user.js and split this into its own package. Then, I can establish some reasonable harnesses & integration tests around it (I neglected to do that in the first pass of this PR b/c lack of precedent).

Please let me know if you need the executable extracted despite my weak protesting. 😉

  • Split out user-info-guessing module
  • Fix "undefined" bug
  • Fix "description" bug
  • Rename scaffold to init
  • Remove superb and replace with awful

Re extra dependencies: I apologize if I go off on a tangent here, but especially since "left-pad-gate", there's been FUD in the JS community around the "hypermodular" nature of many popular Node.js packages.

My stance is: if a dependency is a clear win--as it is in this case, to improve user friendliness--then by all means, it should be included, assuming there's no other obvious deal-breaker.

There are performance gains to keeping the number of bytes loaded by require() to a minimum (e.g., what happens when you have 20 plugins installed and type grunt --help, which is why jit-grunt exists) . In many cases--and in Probot's case, I'd argue--this is a premature optimization.

Are there other reasons why you'd want to reduce the raw number of production dependencies? Or just the direct dependencies of Probot?

@bkeepers
Copy link
Contributor

bkeepers commented May 6, 2017

Please let me know if you need the executable extracted despite my weak protesting.

Nah, it's fine. Let's start with it in core and we can extract it later if it's a pain.

Are there other reasons why you'd want to reduce the raw number of production dependencies? Or just the direct dependencies of Probot?

In general, I prefer to avoid unnecessary dependencies, in the same way I prefer to avoid unnecessary features. No code is better than no code.

@boneskull
Copy link
Contributor Author

In general, I prefer to avoid unnecessary dependencies, in the same way I prefer to avoid unnecessary features. No code is better than no code.

Agreed. It's more about deciding what's necessary and what's not.

@bkeepers I had a couple questions about the results you were getting in this review comment.

@boneskull
Copy link
Contributor Author

OK, I think the missing email issue is fixed.
I've also added some documentation, though not too terribly much.

@boneskull boneskull force-pushed the init branch 2 times, most recently from c1cb57a to 797fc04 Compare May 18, 2017 06:33
@bkeepers
Copy link
Contributor

@boneskull thanks for your work on this, and sorry for letting it languish. After a couple months of thinking about it, I think I'd like to extract this into a create-probot-plugin package, since that seems to be the convention.

Thoughts?

bkeepers added a commit to probot/create-probot-app that referenced this pull request Jul 27, 2017
@bkeepers
Copy link
Contributor

This has been extracted to https://github.com/probot/create-probot-plugin

@boneskull thanks so much for your work on this!

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