crowdin / crowdin-api-client-dotnet

.NET client library for Crowdin API
https://www.nuget.org/packages/Crowdin.Api/
MIT License
52 stars 27 forks source link

feat(screenshots): add the stringIds parameter support #271

Closed EvilKot closed 1 month ago

EvilKot commented 1 month ago

Closes #256

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 63.38%. Comparing base (f45d418) to head (02a681e). Report is 111 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #271 +/- ## =========================================== + Coverage 50.38% 63.38% +13.00% =========================================== Files 277 399 +122 Lines 3688 6084 +2396 Branches 0 501 +501 =========================================== + Hits 1858 3856 +1998 - Misses 1830 2189 +359 - Partials 0 39 +39 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

EvilKot commented 1 month ago

Hello, creating a PR in relation to https://github.com/crowdin/crowdin-api-client-dotnet/issues/256#issuecomment-2411097527

I haven't found any tests related to InternalExtensions, except one checking orderBy string generation. Also there's no tests checking Screenshots.ListScreenshots method (only this ones) so i have no reference/example how to add a new ones.

If it's a mandatory, I'd be really grateful for any help and tips how i could add them. Thank you!

andrii-bodnar commented 1 month ago

Hello @EvilKot, thanks a lot for the contribution!

It would be nice to add tests for the InternalExtensions class and Screenshots.ListScreenshots method. For reference, I think you can use the ProjectsApiTests:ListProjects test case.

The testing approach requires some boilerplate code like resources (e.g. Projects.resx, Projects.Designer.cs). I'm not quite sure how this code is generated.

EvilKot commented 1 month ago

Thank you, @andrii-bodnar!

For Screenshots.ListScreenshots i've added a test based on example you provided. I however manually adjusted corresponsing .resx and .Designer.cs files.

For InternalExtensions i'm not sure i'm going in a right way and if it's actually worth to cover it with tests:

One more thing - i'm a bit struggling with indentation settings used in this repo. Originally my IDE removed whitespaces from blank lines and i not sure how to recover it and if it makes any difference.

🥹

andrii-bodnar commented 1 month ago

@EvilKot thank you!

i'd suggest moving util and .resx files to another directory, hmmm, maybe something like tests/Crowdin.Api.Tests/TestUtils

It definitely makes a lot of sense to move those resources and utilities to TestUtils.

One more thing - i'm a bit struggling with indentation settings used in this repo. Originally my IDE removed whitespaces from blank lines and i not sure how to recover it and if it makes any difference.

I think it would be better to undo this change and avoid trailing spaces.

EvilKot commented 1 month ago

@andrii-bodnar, i'm afraid this PR will become super inflated if i move things into /TestUtils, since it will impact almost every file in tests project 😅

indentation updated 👍

andrii-bodnar commented 1 month ago

@EvilKot sure, it's definitely out of the scope of the current PR

andrii-bodnar commented 1 month ago

@EvilKot I just created a separate issue for that - https://github.com/crowdin/crowdin-api-client-dotnet/issues/272

EvilKot commented 1 month ago

Yay, thank you for walking me through!