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

Topic command #1147

Merged
merged 13 commits into from
Apr 16, 2015
Merged

Topic command #1147

merged 13 commits into from
Apr 16, 2015

Conversation

kyleknap
Copy link
Contributor

This is the second to last PR needed to have the topic help extension fully implemented. This pull request implements the aws help topics and aws help <topic-name> commands. Here is a sample of what aws help topics looks like from the command line currently:
screen shot 2015-02-12 at 7 35 38 pm

Here is what aws help return-codes looks like from the command line currently:

screen shot 2015-02-12 at 7 36 06 pm

This is the second to last pull request because it does not have the following:

  • Notify the user to use aws help topics when you run aws help or a usage error is thrown
  • There is no related topics or related references across other references and topics.

Those will be part of a PR that will fully promote the use of the topic commands, as this PR just exposes but does not document/promote the topic guide quite just yet. Let me know if you would like to see any of the items I listed included in this PR.

cc @jamesls @danielgtaylor

def test_regular_call(self):
with mock.patch('awscli.help.TopicListerCommand.__call__') \
as topics_call:
with mock.patch('awscli.help.TopicHelpCommand.__call__') \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not like the style in which I am trying to test this (way too many mocks). I am trying to test that if nothing is added after aws help, only the general HelpCommand is called. The hard part is that TopicListerCommand and TopicHelpCommand both inherit it from HelpCommand. Any ideas on a cleaner way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Decorators should make this a bit less painful. For example:

@mock.patch('awscli.help.TopicListerCommand.__call__')
@mock.patch('awscli.help.TopicHelpCommand.__call__')
@mock.patch('awscli.help.HelpCommand.__call__')
def test_regular_call(self, help_command, topic_help_command, topic_lister_command):
    self.cmd([], None)
    help_command.assert_called()
    self.assertFalse(topic_lister_command.called)
    self.assertFalse(topic_help_command.called)

Still not perfect, but it's a little cleaner IMO. Also, maybe you don't need the .__call__ part of it?

@danielgtaylor
Copy link
Contributor

Overall LGTM, but I'm not that familiar with the CLI's help system.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.14%) to 92.14% when pulling dc21c9c on kyleknap:topic-command into 2956ab5 on aws:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.14%) to 92.14% when pulling dc21c9c on kyleknap:topic-command into 2956ab5 on aws:develop.

@kyleknap
Copy link
Contributor Author

@jamesls @danielgtaylor

With the recent commits on this pull request, I did the following:

  1. Make suggested changes by @danielgtaylor
  2. Updated the topic commands to use the same style reference linking that was added to the command references.
  3. I exposed aws help topics and aws help <topicname> in aws help. I figured we have been getting enough questions that can be solved by just reading the topics (not even needing the topics to be linked with other topics and references), we should really just get this out the door (meaning promoted through the man pages and html docs). Also, I figured that since we must update our bcdoc requirement by the next release, it would be nice to take advantage of the new released bcdoc changes and expose the topic guide.

self._topic_table = None
self._topic_tag_db = None
self._related_items = [
'AWS CLI Topic Guide (`aws help topics <../topic/index.html>`_)']
Copy link
Member

Choose a reason for hiding this comment

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

I think we're mixing layers here. Prior to this, the help commands knew nothing about how the documentation was actually going to be formatted, nor did they contain any RST markup. This seems like a concern of bcdoc and the clidoc event handlers.

@jamesls
Copy link
Member

jamesls commented Feb 24, 2015

Feature wise, I think this is pretty cool. I'm working on some topic guides now, and so far it seems to be working great. The one thing that I find a little confusing is the usage of multiple categories. I think it will be hard to tell with so few topic guides initially, but I was thinking of categories as the more broad and general concept to associate with the topic. So for example, "return-codes" would not be be part of the "S3 category", I'd only add it to the "General" category. However, if we add something like "tags", then I could see return-codes being tagged with "S3", because there are some S3 specifics to the return code topic guide. However, as it currently stands, it's a little confusing to see return-codes listed under S3, but upon reading the topic, seeing that the topic is about return codes in general.

Code wise, I would prefer that we be more consistent with how the layering currently works with the existing doc components. From what I can tell, prior to this we have:

  • HelpCommands - provide the interface to plug into the command invocations with clidriver (e.g. aws help). These bridge the interface that's needed for command invocation with the documentation system (bcdoc) by managing the doc handler registration, kicking off the event firing, displaying the rendered output.
  • clidocs - Event handlers that take the relevant domain objects (command/arg tables, argument models etc.) and map that to specific documentation. This can turn a command table into the relevant documentation sections and synopsis, etc.
  • bcdoc - The rst specific markup code. It provided a format independent interface for creating markup, e.g. .new_paragraph(), .h2(...), that's used by clidocs.

With this change, the layers seems to be more fuzzy. For example:

  • HelpCommands - These have properties that expose RST markup directly, which seems more appropriate in bcdoc. These also have attributes that don't really need to be exposed on the command objects, they're really only needed for documentation, and seem better suited to live in clidocs, and that's really the thing that needs to know these values in order to generate documentation.
  • clidocs - These bypass the bcdoc layer and just use raw .write() calls that seem like they should be included in bcdoc. We weren't 100% perfect with this before, but it seems like this is writing a lot more direct RST directly.

Overall though, I think we're heading in the right direction.

@kyleknap
Copy link
Contributor Author

kyleknap commented Mar 4, 2015

Talked to @jamesls about this. Here is the summary of each point in the conversation:

  • Help Commands interface -- By adding the ability to have commands past the help command, there is a necessity of these command objects to have a true command_table as opposed to the command_table that is just a copy of the command object that the HelpCommand is documenting. The hard part is that the HelpCommand will now be unique from any other command in that it can both be the end of a chain of commands or in the middle of a chain of commands. To satisfy this need for the HelpCommand there is two options that we discussed: 1) Make a new help command that is like a service/operation/custom command, and have the old help commands just document that. This will take much more time and refactoring, but may prove to be the better option in the long run for the sake of preventing the mixing of functionality of help command with regular commands. 2) Add a new attribute to the current HelpCommand that will allow it the ability to process commands that appear past it. This option is quicker and much less complicated but fuzzies the parity between actual command objects and the help commands. Note that I am probably going to go with this option for now. I am not sure what I am going to name it though.
  • I am going to move all of the logic in the TopicListerCommand and TopicHelpCommand to their corresponding CLI document event handler. This will help maintain the interface for help commands.
  • Move all of the bcdoc related stuff from the CLI and into bcdoc. Note that we will have to do another minor version bump of bcdoc before we can merge the PR.
  • Make aws help topics a flat list with no duplicates topics across categories.

I will send a PR for the the first and third point first, and then update the PR with the fix for the second and fourth point.

@kyleknap
Copy link
Contributor Author

kyleknap commented Mar 6, 2015

Note these test will fail until this PR is merged: boto/bcdoc#34

@kyleknap
Copy link
Contributor Author

@jamesls The PR is now rebased off of the HEAD of develop. Should be good to get looked at.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.13%) to 92.85% when pulling 4407659 on kyleknap:topic-command into 3a4191b on aws:develop.

@jamesls
Copy link
Member

jamesls commented Apr 16, 2015

:shipit: Looks good.

kyleknap added a commit that referenced this pull request Apr 16, 2015
@kyleknap kyleknap merged commit b6d538d into aws:develop Apr 16, 2015
@kyleknap kyleknap deleted the topic-command branch April 16, 2015 23:54
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.

4 participants