OceanNetworksCanada / api-python-client

Provides easy access to ONC data in Python
https://oceannetworkscanada.github.io/api-python-client/
Apache License 2.0
10 stars 9 forks source link

Improve exceptions emitted and handled #18

Closed Jacob-Stevens-Haas closed 9 months ago

Jacob-Stevens-Haas commented 9 months ago

No more naked exceptions, except in one place where it truly is intended (i.e. if something bad happens, warn about it and continue the loop). Exception testing rewritten in pytest, since that's where we're headed and I don't know how to modify robot tests.

Worth noting that documentation on wiki is unclear whether some error codes from server should be 127/129. (e.g. test_raw_bad_filters/test_bad_filters). Other services do not list error codes on the ONC wiki.

kan-fu commented 9 months ago

Monday and Tuesday are our regression test days before the minor release. We will come back to the PR on Wednesday.

BTW I think it would be better to merge your PR first. I need to add some commands in tox for the pytest.

Jacob-Stevens-Haas commented 9 months ago

...We will come back to the PR on Wednesday.

BTW I think it would be better to merge your PR first....

Sorry, I'm confused. Were you approving this PR and saying that we'll come back to your PR on Wednesday, or were you saying that we'll come back to this PR on Wednesday and then your PR?

kan-fu commented 9 months ago

...We will come back to the PR on Wednesday. BTW I think it would be better to merge your PR first....

Sorry, I'm confused. Were you approving this PR and saying that we'll come back to your PR on Wednesday, or were you saying that we'll come back to this PR on Wednesday and then your PR?

Sorry for not making myself clear. It is the latter one. Eli and I are expected to work fulltime on regression test (once every month) for those two days so we don't have time to review the PR. Just don't want to leave you confused when you don't see any comments from us during these two days.

kan-fu commented 9 months ago

From the wiki, it seems that rawdata service does not accept propertyCode unlike scaladata service. That is why it returns 129 (parameter name error) in test_raw_bad_filters, and 127 (parameter value error) in test_no_data_filters. If you want to test 129 in scaladata, you can change propertyCodeto propertyCode1in bad_data_filters.

BTW, did you run black, isort and ruff? There are some warnings, but should be easy to fix. Also I met some conflicts between isort and ruff check I rule. I think we can just remove the I rule from pyproject.toml.

Jacob-Stevens-Haas commented 9 months ago

Pushed new commits with correct linting and formatting. If you want more linting/autoformatting, I'd recommend waiting until #15 is done so I can use the config that PR adds.

kan-fu commented 9 months ago

LGTM! Thanks for your effort. This saves me a lot of trouble when reading except Exception: raise pattern.

One small thing, if you find splitting a long string makes the string look awkward, just add # noqa: E501 to disable the linting. I tend not to be too strict about this line-too-long rule by adding several noqa myself.

For the linting, don't worry about it. We will merge yours first. As I said earlier, something is not set up correctly between isort and ruff check I rule. I will fix it in #19.

Jacob-Stevens-Haas commented 9 months ago

Great, thanks Kan! I tend to be pretty strict about splitting strings unless it's something like a URL that will be copy/pasted a lot.

Am I able to merge this, or are we waiting on @spencerwplovie and @eliferguson?

kan-fu commented 9 months ago

Great, thanks Kan! I tend to be pretty strict about splitting strings unless it's something like a URL that will be copy/pasted a lot.

Am I able to merge this, or are we waiting on @spencerwplovie and @eliferguson?

In ONC we usually need two approvals to merge a PR. They are usually occupied by other tasks. Let's wait for them to review the PR.

eliferguson commented 9 months ago

looks good to me. Ill merge it.

spencerwplovie commented 9 months ago

Apologies for not seeing this sooner, been caught up with other tasks. LGTM.