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

Enhancement: Unify behavior of verbose #41

Merged
merged 5 commits into from
Jun 17, 2020

Conversation

vikin91
Copy link
Contributor

@vikin91 vikin91 commented Jun 16, 2020

This PR changes the following:

  • Use log package for log messages (this allows to respect -v flag setting)
  • Keep using fmt package for commands output (like ls, cat)
  • Improved selected error messages: Not a valid -> Invalid
  • (automated change) Imports were sorted based on type

Pros:

  • This makes errors more visible (they are red),
  • This allows to hide unwanted output (by running command without -v)

Cons:

  • This shows file and line on output. Example: [INFO] append.go:194 Appended values from ...

What do you think?

@fishi0x01 fishi0x01 self-assigned this Jun 16, 2020
@fishi0x01 fishi0x01 added the enhancement New feature or request label Jun 16, 2020
@fishi0x01
Copy link
Owner

I like the idea 👍

I merged your previous PR (whitespaces) to master and updated the branch via GitHub UI. Turns out the whitespace test is failing because the output is missing now. I think adding a -v to the test command should do the trick.

Sorry, I couldn't resist and clicked the Update Branch button - feel free to force-push that away .

@vikin91
Copy link
Contributor Author

vikin91 commented Jun 16, 2020

No problem! I also wanted to click it :) Merging master allowed us to spot an issue with the test. I hope bccbc65 would do the job.

@fishi0x01 fishi0x01 merged commit 69bd9bc into fishi0x01:master Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants