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 logger 3rd party library #94

Closed
vanniktech opened this issue Mar 11, 2022 · 8 comments
Closed

Remove logger 3rd party library #94

vanniktech opened this issue Mar 11, 2022 · 8 comments
Labels
code quality Upgrade code quality (e.g. refactor, test) good first issue Good for newcomers

Comments

@vanniktech
Copy link
Contributor

Quickly looking at the code it seems like there's only one log statement:

logger.info { "skip miss matched row. [csv row num = ${idx + 1}, fields num = ${row.size}, fields num of first row = $fieldsNumInRow]" }

Do we really need to pull an entire library for logging?

implementation("io.github.microutils:kotlin-logging:2.0.11")

I'm an Android user and currently that log would go basically nowhere.

@vanniktech
Copy link
Contributor Author

I'd propose to remove that log statement. If you're using skipMissMatchedRow you're kind of expecting this behavior.

@doyaaaaaken
Copy link
Owner

Thanks for the feedback. 👍 I'll consider it in a short time.

@doyaaaaaken
Copy link
Owner

Hi, @vanniktech, thank you for your useful feedback.

In conclusion, I would like to leave this library dependency as it is.

At first, I think It would be nice to replace the dependency for kotlin-logger with that for slf4j, but I can't do it because kotlin-csv is multiplatform library.
Then, I consider simply removing kotlin-logger, but decide to keep it because logging is an important element for CSV read/write because it is a complex use case that requires debugging.

@vanniktech
Copy link
Contributor Author

kotlin-csv is multiplatform library.

and that's good!

Then, I consider simply removing kotlin-logger, but decide to keep it because logging is an important element for CSV read/write because it is a complex use case that requires debugging.

But then would you want to use a debugger instead of a log statement that goes possibly nowhere, right?

@doyaaaaaken
Copy link
Owner

I see what you're saying, but this log message is very important information for users of the skipMissMatchedRow option.
As of now, I use the kotlin-logger at only one statement, but we would probably use it at other statements in the future.
So, I would like to leave it for now.

@vanniktech
Copy link
Contributor Author

Okay but then would not it be better to have our own Logger interface which - if you want logging, you can pass your own instance and to the logging you'd need - and otherwise you're not bothered by it?

@doyaaaaaken
Copy link
Owner

Sounds like a good idea.
If you have an image of how you would implement it, I would welcome a pull request if you like.

@doyaaaaaken doyaaaaaken added good first issue Good for newcomers code quality Upgrade code quality (e.g. refactor, test) labels Apr 16, 2022
@doyaaaaaken
Copy link
Owner

Released in version 1.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Upgrade code quality (e.g. refactor, test) good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants