SynBioDex / pySBOL2

A pure Python implementation of the SBOL standard.
Apache License 2.0
20 stars 6 forks source link

Improve URL validation in PartShop #92 #344

Closed Benedict-Carling closed 4 years ago

Benedict-Carling commented 4 years ago

This is a pull request adressing:

Improve URL validation in PartShop #92

As suggested I have added two embedded if statements following your suggestions on the issue page. One for using isinstance to raise error if input is not of type str, and second using urlparse to ensure the scheme is one of "http" or "https" as well as a existing netloc. In addition to your current preventing terminal "/" function.

I have tested the current fuction with the follwing urls:

"https://stackoverflow.com" -> Valid "https://stackoverflow.com/" -> Error( terminal forward slash ) "https://" -> Error ( non-existant netloc ) "/stackoverflow.com" -> Error ( invalid scheme ) "[1,2,3]" -> Error ( invalid scheme ) [1,2,3] -> Error ( not string )

If there is any questions, suggestions or misunderstanding of the code on my half let me know as I'd like to continue working on this project in the future πŸš€ Benedict

Fixes #92

tcmitchell commented 4 years ago

Thanks @Benedict-Carling ! We appreciate the contribution!

Would you mind adding a unit test (or a few small tests) in test/test_partshop.py? I think the 6 cases you cite in this pull request would be perfect as a unit test. You can use assertRaises in the unit tests for the exception cases. There are a bunch of examples in the existing unit tests.

Thanks again!

Benedict-Carling commented 4 years ago

Thanks for the fast reply @tcmitchell the unit tests are a good idea πŸ‘ I'll continue to add these, thanks again!

Benedict-Carling commented 4 years ago

Hello πŸ‘‹ @tcmitchell i have added the unit tests for all the examples in my pull request description in my latest commit 7e093d8 If there are any suggestions or improvements let me know!

tcmitchell commented 4 years ago

Fixes #92

tcmitchell commented 4 years ago

@Benedict-Carling thanks again for the contribution. This has been merged.