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

Modified to use new case-sensitive lowercase logrus import #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koshatul
Copy link

@koshatul koshatul commented Feb 9, 2018

Sirupsen changed their github account to sirupsen, so this title-case import will have issues with projects using the new lower case import.

Updated to use the new lower-case version.

@fcvarela
Copy link

This is great, but I'd just like to note that personally I find it preferable for libraries to return errors rather than logging, unless It simply defines a log interface and I can provide my own implementation to harmonise it with the importing app logging.

@koshatul
Copy link
Author

I agree, but I was just getting it working first, I haven't gone back to that project again and I'm not sure this is even maintained anymore.

"github.com/coreos/go-semver/semver"
"github.com/sirupsen/logrus"
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the log package identifier.
It makes it easier to switch for another implementation later on if required.

@bvandewalle
Copy link
Contributor

Depending on the dep management tool used, this PR can indeed be pretty useful. Would like to get this in also @janeczku

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.

3 participants