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

Covenant API #159

Closed
danjagnow opened this issue Nov 22, 2020 · 3 comments
Closed

Covenant API #159

danjagnow opened this issue Nov 22, 2020 · 3 comments
Assignees
Milestone

Comments

@danjagnow
Copy link
Collaborator

danjagnow commented Nov 22, 2020

Add support for the Covenant API, which is a new addition to the Game Data APIs for the Shadowlands expansion. See WoW Shadowlands API Update - Covenants, Soulbinds, & More for details about the new APIs.

@danjagnow danjagnow added this to the 5.0 milestone Nov 22, 2020
@willwolfram18 willwolfram18 self-assigned this Nov 22, 2020
@willwolfram18
Copy link
Collaborator

I'll take a swing at this. Seems like a small and easy enough API set.

@willwolfram18
Copy link
Collaborator

@danjagnow wanted to check and see if you'd be cool with some tweaks I was interested in introducing while unit testing this method. Specifically I was looking at doing the following:

  • Including Fluent Assertions for some validation of model deserializations
  • Tests for different region+locale combos to better exercise some of the other code paths beyond the Region.US and Locale.en_US
    • Not planning to do all combos, but at least get a little more coverage.
  • Change up the test case layout for the Covenant API methods, similar to the DI tests
    • Basically looking at trying to bring a little SRP to the unit tests so that 1 class focuses on testing one method (or in this case pair of methods for the region+locale overload).

Let me know your thoughts, don't want to add too much or create fragmentation if you're not on-board.

@danjagnow
Copy link
Collaborator Author

  • Adding Fluent Assertions would be good. I use it in my professional work, and I like it. I have just been too lazy to add validations beyond the not-null check. This is particularly important after the PR that migrated from Newtonsoft.Json to System.Text.Json. Previously it was possible to set MissingMemberHandling = MissingMemberHandling.Error for debug builds, which made it much easier to discover cases where the C# models lacked a property that was needed for full deserialization of the JSON. See the note about MissingMemberHandling in the migration guidance for details. Hopefully Add support for MissingMemberHandling to System.Text.Json dotnet/runtime#37483 will get addressed in the future.
  • Tests for different combinations of region and locale would be very welcome; again, I've just been too lazy to add these.
  • Changing the test case layout is fine with me. I find that the existing layout is fine for the level of testing that is currently being performed, but having a single test class per API may be too coarse-grained when adding the variants you're proposing. Feel free to change it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants