Bugfix/doublelogger - Githubissues

python-20 / video-downloader

A python application to download videos

GNU General Public License v3.0

10 stars 10 forks source link

Bugfix/doublelogger #57

Closed AlexPHorta closed 4 years ago

AlexPHorta commented 4 years ago

Fixed the double logger issue ( #55 ). Had to move some things around to get everything working.

chonix commented 4 years ago

Er..... even if i often say if this fixes it, keep it. I king of find this fix confusing.

I wouldn't personally use a filename similar to the one that imports the module functionality (import is logger, wouldn't use loggers, its confusing)
Remove commented code if a fix is applied. Helps to read it quicker.
you can define the setup in the same place you import the logging as well. (Why would you separate the configuration from the module import, hence that wont change things)
I hate myself by being harsh to you making these rude comments. But I hope you understand im trying to come up with the logic used here.

cherylli commented 4 years ago

hey works fine for the first 3 lines after pressing OK. But I'm getting

NameError: name 'logger' is not defined

when i press the download button and doing other stuff in gui.py

AlexPHorta commented 4 years ago

* I wouldn't personally use a filename similar to the one that imports the module functionality (import is logger, wouldn't use loggers, its confusing)

Good point, keeping the logger in gui.py results in an import error, due to circular reference. So, I chose to move the logger instantiation to a separate file. I believe it can be placed in helpers.py too.

* Remove commented code if a fix is applied. Helps to read it quicker.

REALLY sorry about that, late night coding.

* you can define the setup in the same place you import the logging as well. (Why would you separate the configuration from the module import, hence that wont change things)

I believe changing the instantiation to helpers.py would solve this, right?

* I hate myself by being harsh to you making these rude comments. But I hope you understand im trying to come up with the logic used here.

You're being objective, that's not harsh nor rude. Thank you.

AlexPHorta commented 4 years ago

hey works fine for the first 3 lines after pressing OK. But I'm getting
NameError: name 'logger' is not defined
when i press the download button and doing other stuff in gui.py

My mistake, REALLY sorry about that too. I'll fix it this night. This tells me we need tests.

RyanSamman commented 4 years ago

I fixed the error, just imported logger from loggers.py your file into gui.py

cherylli commented 4 years ago

We do need test but I've 0 knowledge with tests. I see some github repo run automatic tests on every pull request? And also thats why we cross check before merging

chonix commented 4 years ago

@alexphorta: all good, take your time, thanks for answering the questions @cherylli: I will look onto this with github automation to run tests every time we submit a pull request. I agree on this, see which functions are vital and create unittests for these.

AlexPHorta commented 4 years ago

I fixed the error, just imported logger from loggers.py your file into gui.py

Thanks, Ryan, I won't be able to code for hours.

cherylli commented 4 years ago

Put in helpers sounds fine

AlexPHorta commented 4 years ago

Alright, everything working. Code clean. I tested the program and it's working fine. I placed the logger instantiation in helpers.py and deleted loggers.py.