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

Remove logrus dependency, and add functional options to specify logger #25

Merged
merged 1 commit into from
Sep 29, 2017
Merged

Remove logrus dependency, and add functional options to specify logger #25

merged 1 commit into from
Sep 29, 2017

Conversation

kenshaw
Copy link
Contributor

@kenshaw kenshaw commented Sep 25, 2017

This changeset removes the non-standard dependency to logrus, leaving
the only dependencies used in ansiterm and winterm as Go standard lib
packages. This also has the added benefit of removing a superfulous
package level variable (logger) in both ansiterm and winterm.

The changes made here introduce a backwards compatible change to the
API signatures of ansiterm.CreateParser and winterm.CreateWinEventHandler
by introducing the optional functional parameters pattern on both.
Additionally, both ansiterm and winterm packages receive a WithLogf()
func to specify a logging func to use, which can be passed a func
with the standard Printf signature, ie: func(string, ...interface{}).

This maintains the existing behavior of checking for the environment
variable DEBUG_TERMINAL, and writing to the files ansiParser.log and
winEventHandler.log. For ansiterm/winterm, if a logging func has been
specified as well as having DEBUG_TERMINAL set, then both the respective
*.log file and the passed logging func will receive the data.

Note: for simplicity purposes, the single instance in ansiterm where
there was a "warning" and "error" was logged has been changed to use the
single log, but the messages have been prefixed with WARNING and ERROR
respectively. This was done entirely for simplicity reasons.
Additionally 2 other logging calls have been removed where it was
prudent to do so, and one other logging line was moved to a different
location in order to avoid changing some utility funcs.

This changeset removes the non-standard dependency to logrus, leaving
the only dependencies used in ansiterm and winterm as Go standard lib
packages. This also has the added benefit of removing a superfulous
package level variable (logger) in both ansiterm and winterm.

The changes made here introduce a backwards compatible change to the
API signatures of ansiterm.CreateParser and winterm.CreateWinEventHandler
by introducing the optional functional parameters pattern on both.
Additionally, both ansiterm and winterm packages receive a WithLogf()
func to specify a logging func to use, which can be passed a func
with the standard Printf signature, ie: func(string, ...interface{}).

This maintains the existing behavior of checking for the environment
variable DEBUG_TERMINAL, and writing to the files ansiParser.log and
winEventHandler.log. For ansiterm/winterm, if a logging func has been
specified as well as having DEBUG_TERMINAL set, then both the respective
*.log file and the passed logging func will receive the data.

Note: for simplicity purposes, the single instance in ansiterm where
there was a "warning" and "error" was logged has been changed to use the
single log, but the messages have been prefixed with WARNING and ERROR
respectively. This was done entirely for simplicity reasons.
Additionally 2 other logging calls have been removed where it was
prudent to do so, and one other logging line was moved to a different
location in order to avoid changing some utility funcs.
@Azure Azure deleted a comment from msftclas Sep 27, 2017
@kenshaw
Copy link
Contributor Author

kenshaw commented Sep 28, 2017

I'm confused about the activity from the msftclas and msftgits bots? It removed the cla-signed tag. Does that imply this has no chance of being merged? In anticipation of submitting this PR, I had signed the DocuSign document ahead of time.

Any clarification would be appreciated. Thanks!

@msftclas
Copy link

@kenshaw, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@jstarks
Copy link
Collaborator

jstarks commented Sep 29, 2017

Thanks for the contribution. Looks good.

@jstarks jstarks merged commit d6e3b33 into Azure:master Sep 29, 2017
@kenshaw
Copy link
Contributor Author

kenshaw commented Sep 29, 2017

Thank you, much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants