Conversation
|
@tiagoCMatias Could you create a test case for the same? |
|
@PSNAppz I will give my best to provide tests 👍 |
modules/savedb.py
Outdated
| password = password of MYSQL | ||
| link = URLs from the crawler | ||
| """ | ||
| if database and user and password is None: |
There was a problem hiding this comment.
Don't you think (True is None or False is None) going to evaluate to False and hence in else loop???
There was a problem hiding this comment.
It is just to prevent null values and exits with a message... This can be improved
There was a problem hiding this comment.
Could you switch it to if not database and not user and not password?
There was a problem hiding this comment.
yes, ofc... report here all the necessary refactoring and in aprox. a week I will commit all the changes
modules/savedb.py
Outdated
|
|
||
| #Debug | ||
| #print("Database:", database, "\nuser:",user, "\npass:",password) | ||
| db = MySQLdb.connect(host="localhost", # your host |
There was a problem hiding this comment.
What if connection doesn't happen, error is not caught.
There was a problem hiding this comment.
well, it is true. What would the best fix? a try expect block with a raise in case of error?
There was a problem hiding this comment.
yes, try expect will do
modules/savedb.py
Outdated
| #print(query) | ||
| db.commit() | ||
|
|
||
| except (MySQLdb.Error, MySQLdb.Warning) as e: |
There was a problem hiding this comment.
Since all of the errors invoke the same actions, you can put them all on one line such as
except (MySQLdb.Error, MySQLdb.Warning, TypeError, ValueError) as e
modules/savedb.py
Outdated
| finally: | ||
| cur.close() | ||
| db.close() | ||
| except: |
There was a problem hiding this comment.
Why do you have a blank except right here? You can just use one try-except block instead of a nested one.
There was a problem hiding this comment.
The nested try is to separate errors. You can get an error for not being able to connect to database, or get an error for not being able to insert data into the table
There was a problem hiding this comment.
Yes there should be two completely separate try excepts. for example:
try:
db = MySQLdb.connect(...)
cur = db.cursor()
except:
print("Unable to connect to database")
try:
query = ...
except (MySQLdb.Error, MySQLdb.Warning, TypeError, ValueError) as e:
print(e)```
There was a problem hiding this comment.
Thanks... Nice tip, didn't thought of it
There was a problem hiding this comment.
No problem, I just thought it'd make sense to separate the connection logic from the actual querying logic. Plus if you want blank exceptions to cover as little code as possible since it can be triggered by any error.
modules/savedb.py
Outdated
| db.commit() | ||
|
|
||
| except (MySQLdb.Error, MySQLdb.Warning, TypeError, ValueError) as e: | ||
| print(e) |
There was a problem hiding this comment.
Why not raise error instead of printing and returning None?
There was a problem hiding this comment.
I wasn't considering raising an error for this... If you can't save to the database and raise an error the program will stop... If that is a requirement I can change it but in my opinion, a simple message is better
There was a problem hiding this comment.
It's not a requirement, I was just curious. It's fine as it is though :)
There was a problem hiding this comment.
@KingAkeem want me to raise an error if unable to write to the table??
There was a problem hiding this comment.
No, printing is fine. If we find a need to raise errors over using print statements then we will.
|
@tiagoCMatias Are you still on it? Please see the requested changes. You can join our slack channel for discussions. Please post your email below to send an invitation. |
agrepravin
left a comment
agrepravin
left a comment
There was a problem hiding this comment.
Test cases?? @PSNAppz don't merge unless have extensive test cases, becomes difficult at later point revisiting and writing them
agrepravin
left a comment
agrepravin
left a comment
There was a problem hiding this comment.
please implement requested chnages
|
Yes, I'm still implementing changes... I have other works in my hands right now that's why I'm taking so long to submit new changes. I will try to commit as soon as I can |
modules/savedb.py
Outdated
| db.commit() | ||
|
|
||
| except (MySQLdb.Error, MySQLdb.Warning, TypeError, ValueError) as e: | ||
| print(e) |
There was a problem hiding this comment.
It's not a requirement, I was just curious. It's fine as it is though :)
|
I will start writing tests for this but I will not post it in this issue... maybe open another? |
|
@tiagoCMatias Well I recommend you push the tests here in this branch itself. |
|
@tiagoCMatias Please let me know the status on this PR? |
|
Could you please give a status update? If not, your PR will be closed within 48 hours. |
|
@KingAkeem Should we merge this? |
|
@PSNAppz If you've tested it and it works fine then yes we can merge it. |
|
@tiagoCMatias Merge conflicts needs to resolved? Can you please solve them. |
|
Sorry guys, I'm currently busy and didn't have the time to look at this |
|
@tiagoCMatias It’s ok. Someone else can work on this PR and commit this as it is a mandatory feature. |
|
@tiagoCMatias Thank you for the feature. 👏 |
Revert "Merge pull request #75 from tiagoCMatias/feature-db"
4 participants
#47
Created a simple .env file in root folder and another in modules named savedb.py, also added a new argument when starting the program.
Basically, the operations needed is:
edit the
.envfile with the credentials to access the databasestart the script with
-dblike this:torBot.py -db -u URLThe program will run and in the end, it will store all the links in a table