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

allow command line arguments, limit help regex matching #38

Closed
wants to merge 31 commits into from

Conversation

bfhenderson
Copy link
Contributor

  • change the "^help" regex to "^help$" so other plugins may also start with the word help, i.e. "help me do XYZ" without also matching the general "help" command.
  • help response is now organized by plugin class

NO LONGER IN THIS PR
--- start_will.py and start_dev_will.py accept optional arguments to override plugins and templates directories. ---

./start_dev_will.py --help
usage: start_dev_will.py [-h] [-p [PLUGINS]] [-t [TEMPLATES]]

Start Will the Chatbot

optional arguments:
  -h, --help            show this help message and exit
  -p [PLUGINS], --plugins [PLUGINS]
                        comma seperated plugin directory paths (default:
                        ['plugins'])
  -t [TEMPLATES], --templates [TEMPLATES]
                        comma seperated template directory paths (default:
                        ['templates'])

description='Start Will the Chatbot',
formatter_class = argparse.ArgumentDefaultsHelpFormatter)
parser.add_argument('-p', '--plugins', type=csv, nargs='?', help="comma seperated plugin directory paths", default=["plugins",])
parser.add_argument('-t', '--templates', type=csv, nargs='?', help="comma seperated template directory paths", default=["templates",])
Copy link

Choose a reason for hiding this comment

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

Similarly here - can we add "paths" or "dirs" to the parameter name?

@skoczen
Copy link
Owner

skoczen commented Jun 4, 2014

Hey @bfhenderson,

Thanks for the PR! Big-picture, this is one of those cases where I realize a) I need to do a much better job communicating future plans, and b) open-source development is sometimes just messy.

Your pain-points for the plugins and template config are felt by me and lots of other users. Right now, the proposed path forward is #37. One thing I also want to avoid with will is having two ways to do the same thing, and from how I see the landscape now, the config file format of 37 feels like a better solve. Please take a look and see if you agree, or if there are aspects to the CLI approach that accomplish something that's missing in the config. Input welcome!

I'm also +1 to a better CLI for will, generally - see the --dev flag on #31 / #37, so I think a lot of what's here is helpful in the long term.

Generally +1 to the PR comments by dpoirer, but before taking on all of those, Please take a look at 37 and the big picture, and let me know your thoughts! Apologies for not doing better communication on this - if you have ideas for how to and where we could communicate this better, I'm all ears!

-Steven

bot = WillBot(plugins_dirs=["plugins",], template_dirs=["templates",])
bot.bootstrap()
```
Where `run_will.py` can be called with or without arguments to bootstrap an instance of WillBot.
Copy link
Owner

Choose a reason for hiding this comment

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

Note: I'd prefer to keep something like this in docs - the idea being that you could implement will within another project as pure python, instead of having to call him via the CLI.

@skoczen
Copy link
Owner

skoczen commented Jun 4, 2014

Also, re: the help plugin stuff, if it's easy to separate that out from the CLI stuff, that'd be great - that stuff doesn't conflict with #37 at all, and makes sense to get merged in without having to talk through the big config space!

print "\n %s:" % plugin_info["name"]
print "\n %s: (%s)" % (plugin_info["name"], plugin_info["class"].__doc__)
self.help_files[plugin_info["name"]] = []
plugin_help = self.help_files[plugin_info["name"]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry dpoirier, looks like the act of updating the code destroys comments. Anyways plugin_help should be a more accurate name.

Copy link
Owner

Choose a reason for hiding this comment

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

They're still there, but as "commented as an outdated diff" up above, when in conversation view. :)

@bfhenderson
Copy link
Contributor Author

No worries, config file looks like a fine solution. I'll just do some
hardcodes until 50 rolls out. Will submit a new PR.

On Wed, Jun 4, 2014 at 10:56 AM, Steven Skoczen notifications@github.com
wrote:

Hey @bfhenderson https://github.com/bfhenderson,

Thanks for the PR! Big-picture, this is one of those cases where I realize
a) I need to do a much better job communicating future plans, and b)
open-source development is sometimes just messy.

Your pain-points for the plugins and template config are felt by me and
lots of other users. Right now, the proposed path forward is #37
#37. One thing I also want to
avoid with will is having two ways to do the same thing, and from how I see
the landscape now, the config file format of 37 feels like a better solve.
Please take a look and see if you agree, or if there are aspects to the CLI
approach that accomplish something that's missing in the config. Input
welcome!

I'm also +1 to a better CLI for will, generally - see the --dev flag on
#31 #31 / #37
#37, so I think a lot of
what's here is helpful in the long term.

Generally +1 to the PR comments by dpoirer, but before taking on all of
those, Please take a look at 37 and the big picture, and let me know your
thoughts! Apologies for not doing better communication on this - if you
have ideas for how to and where we could communicate this better, I'm all
ears!

-Steven


Reply to this email directly or view it on GitHub
#38 (comment).

@bfhenderson
Copy link
Contributor Author

Oops, thought this PR had been closed but was mistaken. Anyways the outstanding changes are in and this request is now only about the help command. It is easy enough to toss back in argparse if/when the time comes.

@skoczen
Copy link
Owner

skoczen commented Jun 5, 2014

Awesome, thanks for making those changes - going to play with this version of help this AM and see functionally how it feels! (Have some wonders if as a non-technical user I'm more confused by the plugin list, but want to actually live with it before making a decision!)

@bfhenderson
Copy link
Contributor Author

Great, yeah let me know how it feels. Fundamentally what I've got to solve
is grouping help about related commands together. There are a pair of
plugins with maybe a dozen related commands each. When their help is all
mixed together it became much harder to understand how to use the system.

An improvement might be to use the class' doc parameter if not None
instead of its name. Doing such would hide end users from potentially
overly-technically plugin names, and allow will default plugins that span
multiple classes to be carried under some common monicker like "Common
Commands"

On Thu, Jun 5, 2014 at 11:03 AM, Steven Skoczen notifications@github.com
wrote:

Awesome, thanks for making those changes - going to play with this version
of help this AM and see functionally how it feels! (Have some wonders if as
a non-technical user I'm more confused by the plugin list, but want to
actually live with it before making a decision!)


Reply to this email directly or view it on GitHub
#38 (comment).

@skoczen
Copy link
Owner

skoczen commented Jun 7, 2014

So, the team here thought that with the number of plugins we had, the list got too long, but agreed generally that grouping itself was good. So, here are my thoughts/plan:

a) I'm going to pull in the ^help fix now, since that's awesome.
b) I think the new structure that comes with #37 (plugins will be grouped into logical modules) may fit really well with what you're doing here, and once that hits, that should be a great foundation for this code.

So, with that, I'd say let's leave this PR open, and once 0.5 hits, revisit. How's that sound to you?

@skoczen
Copy link
Owner

skoczen commented Jun 7, 2014

Help fix has been released with 0.4.10!

@bfhenderson
Copy link
Contributor Author

Cool! Yeah we can revisit after 0.5 for the help response formatting, will
be keeping it in our fork. Ultimately I think there will be a lot of
considerations going into help, mulling over #41 now.

I'll try to move feature brainstorming/communication more into github
issues before creating PR's directly in the future. That should help keep
the PRs better in tune with upstream goals.

On Fri, Jun 6, 2014 at 7:48 PM, Steven Skoczen notifications@github.com
wrote:

Help fix has been released with 0.4.10!


Reply to this email directly or view it on GitHub
#38 (comment).

@bfhenderson
Copy link
Contributor Author

I'm closing this PR. Can create a new one for some of the other features, but for now its all kind of waiting on what 0.5 contains.

The next PR will

  • add help search
  • provide a user_help decorator, and allow for nested decorators
  • decouple some of the assumptions made in the help system
  • deprecates doc end-user documentation (but provides backward compatibility)

Considerations I'll need to see before issuing it:

  • admin commands - how closely aligned are they with "programmer" commands, can we get rid of "programmer" help?
  • new plugin module system, and generally what that means to help

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