ptrv / gpx2spatialite

GNU General Public License v3.0
23 stars 8 forks source link

Check if database table "users" exists. #14

Closed johnjohndoe closed 10 years ago

johnjohndoe commented 10 years ago

If you run gpx2spatialite -d <path/to/database> -u <user_id> <path/to/gpx> without knowledge of gpx2spatialite_create_db gpx2spatialite fails with an error. With this commit the user is told what to do.

ptrv commented 10 years ago

Thanks! It would be nice though, if the check would include all needed tables, not only users. And unit tests ;)

johnjohndoe commented 10 years ago

I would not test for other tables since the part of the code only deals with the users table. I added a test for the True case. Can you assist on the False case? I thought of adding this function to the Db class.

def remove_users_table():
    sql = "drop table users"
    self.cursor.execute(sql)

I am not sure if this is a good idea. Nonetheless, I need a database which has no users table. Adding another .sqlite file feels too much.

belasco commented 10 years ago

I also think that testing for other tables might be redundant since the use case I can imagine is that someone makes a spatialite database in another way and it doesn't have the schema we designed. In this case, the lack of a users table would tell you that. @johnjohndoe What is the scenario for not having a users table? Can you make a dummy user?

johnjohndoe commented 10 years ago

@belasco Thanks for your awake eyes. We both did not see the scrumbled sentence. The scenario is as I stated: I executed gpx2spatialite without running gpx2spatialite_create_db before. That is why I would guide the uninformed user.

belasco commented 10 years ago

@johnjohndoe No problem, glad to help. I meant what is the scenario for needing ".. a database which has no users table." It doesn't seem to me like you mean that you created a database in the wrong way, more that you want to build a database without a users table and I was asking what this scenario would be and why you would need to add a 'remove_users_table' function?

johnjohndoe commented 10 years ago

@belasco I would only add the function to being able to test the lack of the users table. Other than that I do not have a use case.

ptrv commented 10 years ago

I would not test for other tables since the part of the code only deals with the users table.

The code at that particular part of course only handles the user table, but running the script with other tables missing would also prevent the script to run correctly. Anyways, I think its ok to only check for the user and then printing some hints.

I added a test for the True case. Can you assist on the False case?

I think it's ok to test for a non existing table like users-not-existing or something similar. No need to remove the table.

Please add the false test, fix that too long line and squash.

johnjohndoe commented 10 years ago

I think it's ok to test for a non existing table like users-not-existing or something similar. No need to remove the table.

I like the idea. Done.

Please add the false test, fix that too long line and squash.

Done.

Please use .format instead of % for future python3 support.

Done.

Please use snake_case: test_check_if_table_exists

Done. I was following your test function schema though.

Why not return value 1?

Changed exit code to 1.

ptrv commented 10 years ago

Done. I was following your test function schema though.

The schema is: test_[function_name]. I think i follow the schema too.

ptrv commented 10 years ago

Thanks!