-
Notifications
You must be signed in to change notification settings - Fork 11
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
Refactor learn markdown subcommands into Cobra Commands with tests #128
Conversation
NOTE: This refactor leaves the entire CLI usage exactly the same. The only difference is that the help messages for learn markdown are much more thorough. This refactor moves all learn markdown commands into their own files and Cobra Commands. The common flags for all markdown commands are put into the PersistentFlags of the markdown command object. Those flags are then accessible to all child commands. The newly-implemented flags for questions/challenges are now only defined for the markdown subcommands in the "questions" group. The markdown subcommand-specific help messages now contain the same introductory text that can be found in the Learn Documentation cohort. (https://learn-2.galvanize.com/cohorts/667) The orthogonal interest of how to write output to a destination (clipboard, stdout, append) has also been refactored into its own package (templates). This allows for independent testing of the methods of writing. The refactor necessitated the upgrade of the Cobra library to the most recent version. Because of this refactor, it is now possible to test markdown subcommands in isolation, not only their configuration, but also the way the execute. This has allowed for nearly 100% coverage of all Markdown-related code in the CLI.
@pgrunde Looks like the mock framework for testing is only supported by 1.21 and 1.22. So, I'm going to push a new workflow. |
Also, moving clipboard test to integration test. Can be run locally on a machine with a clipboard with: go test ./... -tags=integration |
@pgrunde It's now ready for you to review. Sorry about the test thing. There were no clipboard tests previously, and "it works on my machine" proved sufficient until the push. |
Here's the coverage report with the integration test included.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has allowed for nearly 100% coverage of all Markdown-related code in the CLI.
This is stellar, thanks! I've always wanted to refactor the markdown generation and add actual test coverage. Really appreciate the work here.
- QA'd Question creation md commands for equivalent behavior to master ✅
- QA'd File creation md commands for equivalent behavior ✅
- QA'd Yaml creation commands for equivalent behavior ❌
- QA'd Other commands for equivalent behavior ✅
This was performed by comparing output for each md subcommand as a diff, where one is output from glearn-cli
installed from this branch command-refactor
, and the other ./glearn-cli
is a from a build of current master.
Using this technique I discovered that the unit description file is not properly interpolating a UID into both minified and maxified (new word!) template.
diff <(./glearn-cli md dsy -om) <(glearn-cli md dsy -om)
Or simply running the md dsy -m
and see that it displays the %s
instead of the UID.
I'm guessing this is because the yaml
package root.go file is using templates.NewStaticTemplate
when the description.yaml generator actually makes use of a generated UID. It doesn't seem appropriate though to use the full-on templates.NewAttributeTemplate
here since the dsy
subcommand will never have explanations, hints, rubrics, etc. Might be worth a constructor for just the id template? Of course it ruins the elegance of createYamlCommand
always getting to use one template type. As with life and code, complexity ruins everything.
If it helps, here is all the commands at once for the minified output. The output for each should only show a diff of the uid fields, and it should show separate uid values on each.
diff <(./glearn-cli md cp -om) <(glearn-cli md cp -om)
diff <(./glearn-cli md fh -om) <(glearn-cli md fh -om)
diff <(./glearn-cli md in -om) <(glearn-cli md in -om)
diff <(./glearn-cli md ls -om) <(glearn-cli md ls -om)
diff <(./glearn-cli md rs -om) <(glearn-cli md rs -om)
diff <(./glearn-cli md sv -om) <(glearn-cli md sv -om)
diff <(./glearn-cli md cb -om) <(glearn-cli md cb -om)
diff <(./glearn-cli md cs -om) <(glearn-cli md cs -om)
diff <(./glearn-cli md ja -om) <(glearn-cli md ja -om)
diff <(./glearn-cli md js -om) <(glearn-cli md js -om)
diff <(./glearn-cli md mc -om) <(glearn-cli md mc -om)
diff <(./glearn-cli md nb -om) <(glearn-cli md nb -om)
diff <(./glearn-cli md or -om) <(glearn-cli md or -om)
diff <(./glearn-cli md pg -om) <(glearn-cli md pg -om)
diff <(./glearn-cli md pr -om) <(glearn-cli md pr -om)
diff <(./glearn-cli md py -om) <(glearn-cli md py -om)
diff <(./glearn-cli md rb -om) <(glearn-cli md rb -om)
diff <(./glearn-cli md sa -om) <(glearn-cli md sa -om)
diff <(./glearn-cli md sq -om) <(glearn-cli md sq -om)
diff <(./glearn-cli md tl -om) <(glearn-cli md tl -om)
diff <(./glearn-cli md up -om) <(glearn-cli md up -om)
diff <(./glearn-cli md co -om) <(glearn-cli md co -om)
diff <(./glearn-cli md dc -om) <(glearn-cli md dc -om)
diff <(./glearn-cli md cfy -om) <(glearn-cli md cfy -om)
diff <(./glearn-cli md cry -om) <(glearn-cli md cry -om)
diff <(./glearn-cli md dsy -om) <(glearn-cli md dsy -om)
diff <(./glearn-cli md tpr -om) <(glearn-cli md tpr -om)
The description.yaml file needs UID replacement. This commit addresses that. [x] Unit test created to demonstrate the problem [x] YAML command parameter added to indicate the need of a UID [x] `createYamlCommand` updated to use the new flag [x] New flag explicitly set in all YAML commands
@pgrunde Thanks for those commands and your diligence! Commit 786f5ca address the description.yaml discrepancy. Address error in descyaml generator The description.yaml file needs UID replacement. This commit addresses that.
The test reads like this:
re := regexp.MustCompile(`UID: [[:alnum:]]{8}-[[:alnum:]]{4}-[[:alnum:]]{4}-[[:alnum:]]{4}-[[:alnum:]]{12}`)
if tc.hasUID {
rendered := template.Render()
if !re.MatchString(rendered) {
t.Error("Did not find a UID GUID in the rendered file:\n" + rendered)
}
} |
NOTE: This refactor leaves the entire CLI usage exactly the same. The only difference is that the help messages for learn markdown are much more thorough.
This refactor moves all learn markdown commands into their own files and Cobra Commands. The common flags for all markdown commands are put into the PersistentFlags of the markdown command object. Those flags are then accessible to all child commands.
The newly-implemented flags for questions/challenges are now only defined for the markdown subcommands in the "questions" group.
The markdown subcommand-specific help messages now contain the same introductory text that can be found in the Learn Documentation cohort. (https://learn-2.galvanize.com/cohorts/667)
The orthogonal interest of how to write output to a destination (clipboard, stdout, append) has also been refactored into its own package (templates). This allows for independent testing of the methods of writing.
The refactor necessitated the upgrade of the Cobra library to the most recent version.
Because of this refactor, it is now possible to test markdown subcommands in isolation, not only their configuration, but also the way the execute. This has allowed for nearly 100% coverage of all Markdown-related code in the CLI.
Example output
Here's some example output for the markdown command.
learn md
This uses Cobra Command groups to group the different Markdown subcommands into different sections of the help message.
learn md cs --help
Here's the help message for the custom snippet subcommand. You can see a couple of things of interest: