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

Nolog #71

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Nolog #71

wants to merge 3 commits into from

Conversation

raffaem
Copy link
Contributor

@raffaem raffaem commented Dec 6, 2023

No description provided.

@MaartenGr
Copy link
Owner

Thank you for the PR. I believe that when you put the logger inside the class, there will be issues with multiple loggers having the same name. For instance, if you create multiple instances of different PolyFuzz classes, multiple copies of loggers would be run.

@raffaem
Copy link
Contributor Author

raffaem commented Dec 6, 2023

There are no multiple loggers with the same name.

Two loggers with the same name are the same logger, just assigned to two different variables.

This is why it's called logging.getLogger, not logging.makeLogger.

@MaartenGr
Copy link
Owner

I'll have to test it first before merging. I remember there being an issue with initializing it within the class itself but I am not sure. I might have some time for it this week.

@MaartenGr
Copy link
Owner

I just tried it out and I get indeed duplicate logging when I use your PR. When I install your PR and run the following, I get duplicate logging:

from polyfuzz import PolyFuzz

from_list = ["apple", "apples", "appl", "recal", "house", "similarity"]
to_list = ["apple", "apples", "mouse"]

model = PolyFuzz("TF-IDF", verbose=True)
model.match(from_list, to_list)

model1 = PolyFuzz("TF-IDF", verbose=True)
model1.match(from_list, to_list)

model2 = PolyFuzz("TF-IDF", verbose=True)
model2.match(from_list, to_list)

However, this is not the case when I just do pip install polyfuzz.

We add a new handler only if the logger doesn't already have one
@raffaem
Copy link
Contributor Author

raffaem commented Dec 6, 2023

fixed

@raffaem
Copy link
Contributor Author

raffaem commented Dec 15, 2023

I just tried it out and I get indeed duplicate logging when I use your PR. When I install your PR and run the following, I get duplicate logging:

@MaartenGr I fixed the bug

@MaartenGr
Copy link
Owner

@raffaem Thanks for the fix. I will have to test it out later this week or next.

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.

None yet

2 participants