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

Added url validation for starting with http, https #20

Merged
merged 13 commits into from Feb 22, 2022

Conversation

WingCode
Copy link
Contributor

@WingCode WingCode commented Feb 8, 2022

The validation (starting with http, https) is expected by the docstring but not enforced.
It might be helpful to fail early and fast since there might be some prior resource intensive steps before utilising the IQM client.

@hay-k hay-k self-requested a review February 10, 2022 07:23
src/iqm_client/iqm_client.py Outdated Show resolved Hide resolved
src/iqm_client/iqm_client.py Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Show resolved Hide resolved
Copy link
Contributor

@hay-k hay-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will also need to add a unit test case to cover this new exception. Let me know if you need any help.

@hay-k
Copy link
Contributor

hay-k commented Feb 10, 2022

@WingCode Thanks for your pull request. I have reviewed it and requested some changes above.

WingCode and others added 4 commits February 11, 2022 12:15
Refactor changelog for cleaner statement from review

Co-authored-by: Hayk Sargsyan <52532457+hay-k@users.noreply.github.com>
Improved url schema check with "http:", "https:" and better exception message from review

Co-authored-by: Hayk Sargsyan <52532457+hay-k@users.noreply.github.com>
@WingCode WingCode requested a review from hay-k February 20, 2022 11:58
Copy link
Contributor Author

@WingCode WingCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed review comments. Fix changelog, IQM client, added tests.

tests/test_iqm_client.py Outdated Show resolved Hide resolved
tests/test_iqm_client.py Outdated Show resolved Hide resolved
WingCode and others added 2 commits February 22, 2022 14:50
Added empty line at end of file to comply with PEP 8 standard

Co-authored-by: Hayk Sargsyan <52532457+hay-k@users.noreply.github.com>
@WingCode WingCode requested a review from hay-k February 22, 2022 10:55
@hay-k hay-k merged commit 2998dba into iqm-finland:main Feb 22, 2022
@WingCode WingCode deleted the add-url-validation-iqmclient branch February 22, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants