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.

Sign up for GitHub

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

Jump to bottom

Remove commands without code blocks #70

Merged
merged 5 commits into from
Aug 10, 2020
Merged

Remove commands without code blocks #70

merged 5 commits into from
Aug 10, 2020

Conversation

twitu
Copy link
Contributor

@twitu twitu commented Jul 31, 2020

Which issue does this fix?

Closes #65

Describe the solution

Recursively remove commands that do not have any code block and have no subcommands. Retain root level command with level == 1.

@twitu twitu requested a review from jacobdeichert as a code owner July 31, 2020 06:00
@twitu
Copy link
Contributor Author

twitu commented Jul 31, 2020

If this recursive pruning of the command tree is an acceptable solution. I can add a few test cases and fix ones that conflict with this change.

@jacobdeichert
Copy link
Owner

@twitu thanks for the PR! I like the solution, it's very clean 👍

If you can add some tests, that would be great.

Also looks like one test is failing, does_not_add_verbose_optional_flag_to_command_with_no_script but we can delete that test as it's no longer needed with this fix.

@twitu
Copy link
Contributor Author

twitu commented Aug 2, 2020

I have modified two test cases to fit pruning of commands. However there are also three tests in subcommands_test.rs that are failing. I am not sure if these tests are relevant any longer. How to handle them?

Also I have a question regarding additional tests. Should I write tests that simulate user input or ones that simply check the command_tree structure for retained commands? Also which file is the most appropriate place to put these tests?

@twitu
command can exist without executor
32c4d28 
This is a design decision. It is possible that the user might have
missed giving the executor for a command. In such a case it is better
to retain the command and provide a helpful error message indicating
the problem. Telling the user that the executor is missing is better
than saying the command does not exist (because it has been pruned)
brandonkal
brandonkal suggested changes Aug 5, 2020
View reviewed changes
tests/arguments_and_flags_test.rs Outdated Show resolved Hide resolved
brandonkal
brandonkal reviewed Aug 6, 2020
View reviewed changes
src/command.rs Outdated Show resolved Hide resolved
@jacobdeichert jacobdeichert merged commit f5460d9 into jacobdeichert:master Aug 10, 2020
@jacobdeichert
Copy link
Owner

@twitu this is great! I'll get a new version released soon.

@jacobdeichert
Copy link
Owner

I'll release this after I figure out this #64 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonkal brandonkal brandonkal requested changes

@jacobdeichert jacobdeichert jacobdeichert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't add a command for a heading that has no code blocks
3 participants
@twitu @jacobdeichert @brandonkal