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

Add missing jo options #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hartzell
Copy link

Add support for several options that jo supports but gjo does not

  • -B -- disable treating true/false as bool
  • -e -- empty stdin is not an error
  • -n -- ignore keys with empty values
  • read from stdin if not args are supplied

It adds code to handle the various features, modifies the run function
to take an io.Reader as an argument for testability, and calls the run
function with os.Stdin in main.

It modifies the TestRun test:

  • The test cases have been updated:
    • renamed fields to be clear about meaning
    • only non-zero test values need to be supplied in the test case
    • provides stdin as a string.Reader
    • provides want_stdout to hold the expected output
  • Tests have been added for new features

george.hartzell added 2 commits December 29, 2020 11:10
Don't risk committing the result of `go build`, ignore it via
.gitignore.
Add support for several options that jo supports but gjo does not

- -B -- disable treating true/false as bool
- -e -- empty stdin is not an error
- -n -- ignore keys with empty values
- read from stdin if not args are supplied

It adds code to handle the various features, modifies the run function
to take an io.Reader as an argument for testability, and calls the run
function with os.Stdin in main.

It modifies the TestRun test:

- The test cases have been updated:
    - renamed fields to be clear about meaning
    - only non-zero test values need to be supplied in the test case
    - provides stdin as a string.Reader
    - provides want_stdout to hold the expected output
- Tests have been added for new features
main.go Outdated Show resolved Hide resolved
main_test.go Outdated
Comment on lines 176 to 178
for test_flag, value := range _flags(test.flags) {
flag.CommandLine.Set(test_flag, value)
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
for test_flag, value := range _flags(test.flags) {
flag.CommandLine.Set(test_flag, value)
}
for flag, value := range _flags(test.flags) {
flag.CommandLine.Set(flag, value)
}

Copy link
Author

Choose a reason for hiding this comment

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

Can't use flag, the LSP output sums it up nicely:

renaming this var "test_flag" to "flag" would shadow this reference to the imported package name declared here

main_test.go Outdated Show resolved Hide resolved
@@ -133,7 +149,7 @@ func doVersion() error {
})
}

func run() int {
func run(stdin io.Reader) int {
Copy link
Owner

Choose a reason for hiding this comment

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

For testability, I think we should accept args and return result.

func run(args []string) string

Copy link
Author

Choose a reason for hiding this comment

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

Happy to make changes here, but I don't quite understand what you want this to look like. Can you explain more fully?

  1. I think you're suggesting to pass in the args instead of referring to the global flag.Args(). Are you suggesting parsing the args and etc.. in main?
  2. I find passing in an io.Reader instead of relying on os.Stdin or mocking it or using real files makes the table driven tests in main_test.go:TestRun cleaner (possible).
  3. What result do you want to return? It's currently returning success/failure, which is easily testable in main_test.go:TestRun, along with the output and stderr.

- use camelCase instead of snake_case for variable names (sorry, I've
  been dealing with a bunch of python...).
- rename functiont that returns default flags to defaultFlags.
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