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

change InputError to KeyError #412

Merged
merged 10 commits into from May 6, 2024
Merged

change InputError to KeyError #412

merged 10 commits into from May 6, 2024

Conversation

Daafip
Copy link
Collaborator

@Daafip Daafip commented May 2, 2024

Missed the Bart's last comment in #407 :||, this works better if we can actually see the sonar could test

@Daafip Daafip requested a review from BSchilperoort May 2, 2024 14:00
@Daafip Daafip marked this pull request as ready for review May 2, 2024 14:00
@Daafip
Copy link
Collaborator Author

Daafip commented May 3, 2024

I was hoping more for something like a webpage listing the basin_id's together with river/basin names, country, etc. I guess that doesn't exist?

Shall I also add a link to these maps with the basin_ids? https://github.com/Daafip/caravan-map

@BSchilperoort
Copy link
Contributor

BSchilperoort commented May 3, 2024

I was also working on fixing some things yesterday, see 876efe8
Perhaps you can merge those changes. I was making the get_basin_id function a classmethod, as that's how it was described in the docstring.

Shall I also add a link to these maps with the basin_ids? https://github.com/Daafip/caravan-map

Could be nice, yes. But perhaps better if you can host it on github pages. Perhaps moving it to the ewatercycle org might also be better. Also see the issue I opened Daafip/caravan-map#1

@Daafip
Copy link
Collaborator Author

Daafip commented May 3, 2024

But perhaps better if you can host it on github pages. Perhaps moving it to the ewatercycle org might also be better

https://www.ewatercycle.org/caravan-map/ Done!

Do you have any suggestions for the index page?

@BSchilperoort
Copy link
Contributor

Do you have any suggestions for the index page?

I think if you copy part of the README there that would be good. Some explanation why the map exists (to explore and find basin IDs). And put in some relevant links to e.g. the ewatercycle documentation.

@Daafip
Copy link
Collaborator Author

Daafip commented May 3, 2024

function a classmethod

How do I then mock the function if its a class method?

@BSchilperoort
Copy link
Contributor

function a classmethod

How do I then mock the function if its a class method?

I believe just with mock.patch.
https://stackoverflow.com/a/53485182

@Daafip
Copy link
Collaborator Author

Daafip commented May 3, 2024

function a classmethod

How do I then mock the function if its a class method?

I believe just with mock.patch. https://stackoverflow.com/a/53485182

Ah, issue with my local unit testing and having the correct version

@Daafip Daafip force-pushed the fix_sonar_cloud_add_caravan branch from f9ed563 to aa97515 Compare May 3, 2024 15:06
@Daafip
Copy link
Collaborator Author

Daafip commented May 3, 2024

Tried to fix the test coverage but don't think a higher coverage is easily met as most of uncovered is downloading files..

Copy link
Contributor

@BSchilperoort BSchilperoort left a comment

Choose a reason for hiding this comment

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

Just two minor comments. Feel free to merge once these are resolved.

With the downloading writing sensible tests can be difficult. But I think this file is sufficiently covered now. The file has an 85% coverage, sonarcloud just doesn't like that some of the changed lines in this PR aren't covered (the measure is per PR, not per file...)

src/ewatercycle/_forcings/caravan.py Outdated Show resolved Hide resolved
tests/src/base/test_forcing.py Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented May 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
75.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@Daafip Daafip merged commit b437045 into main May 6, 2024
3 of 4 checks passed
@Daafip Daafip deleted the fix_sonar_cloud_add_caravan branch May 6, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants