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 eprintln! usage from this library? #88

Closed
ArniDagur opened this issue Apr 11, 2020 · 3 comments
Closed

Remove eprintln! usage from this library? #88

ArniDagur opened this issue Apr 11, 2020 · 3 comments
Labels
enhancement New feature or request

Comments

@ArniDagur
Copy link
Contributor

In the following Python code, I call Engine's add_filter_list method on the string contents of each downloaded block list.

            try:
                download.fileobj.seek(0)
                text = io.TextIOWrapper(download.fileobj, encoding="utf-8")
                self._engine.add_filter_list(text.read())
                text.close()
            except UnicodeDecodeError:
                message.info("Block list is not valid utf-8")
            finally:
                download.fileobj.close()

When I run said code on a few popular blocklists, the console is filled with Filter already exists:

...
Filter already exists
Filter already exists
Filter already exists
Filter already exists
Filter already exists
Filter already exists
17:14:30 INFO: Block lists have been successfully imported

Those particular messages come from src/engine.rs#L166 where the library prints to the console in case of an error. I'm personally not a fan of this kind of "logging", since there's no way for me as a consumer of the library to tell it not to print to the console.

@AndriusA Do you agree that another method of logging errors should be used? If so, what would be your preferred alternative approach? I'd be happy to make a PR.

@ArniDagur
Copy link
Contributor Author

@AndriusA Hey! I have time over the weekend to work on this. Just tell me how you want it to be done, if at all.

@antonok-edm
Copy link
Collaborator

antonok-edm commented Jul 1, 2020

@ArniDagur I agree, better methods of error logging sound like a great idea and I'd be happy to improve adblock-rust in this regard.

As it stands, many of the Engine APIs either return nothing, or are builder-style methods that return another mutable reference to self. I think the best way to go about this would be:

  • For methods that return nothing, return a Result<(), E> instead, where E is a custom error type denoting the problems that can occur for that particular method.
  • For builder-style methods, it'd be impractical to return a Result. I'm not sure how commonly these are actually chained together, but I'm not opposed to doing away with the builder-style altogether. Then these can fit in with the first case as well. The builder-style methods are gone as of v0.3.

Does that sound reasonable?

@antonok-edm antonok-edm added the enhancement New feature or request label Jul 9, 2020
@antonok-edm
Copy link
Collaborator

Just pulled out the last few eprintln cases with 88c1a6b, so I'll consider this fixed!

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

No branches or pull requests

2 participants