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

Model & Client Audit #43

Merged
merged 25 commits into from
Aug 10, 2019
Merged

Model & Client Audit #43

merged 25 commits into from
Aug 10, 2019

Conversation

nicholi
Copy link
Contributor

@nicholi nicholi commented Aug 10, 2019

Addresses #36

Went over all Request/Response models and Client methods against current API docs. Will be adding comments to particular code sections with reasoning behind changes. Most of the changes were creating an explicit Request or Response class (if one was used for both), or an explicit Update class, changing ValueType properties in Request classes to be Nullable if they were optional in particular request (and not marked as Nullable in Response because they are guaranteed to be present), and found some classes which seemed to be very incomplete (TagResources and ForwardingRules).

I'm going to add a Migrating to version 3.x section to the README.md as well, with summary of class/property changes that break API.

Add ForwardingRulesList, the list request used in adding/removing.
Explicit ForwardRule request (1 optional value type) and response class.
LoadBalancer request modified, 2 optional value types.
Forgot to removed un-necessary JsonPropertAttributes from response
…move un-necessary JsonPropertyAttributes from response class.

Rename type->Type
…Create method.

Move ImageType enum to its own file.
Simplify GetAll switch logic.
There is no UpdateFingerprint to Test.
4 optional value type properties.
…s for Tag/Untag methods.

Renamed request Resource->TagResource and move into its own file.
Rename Resource->TagResourcesStatistics, strongly typed respone class. Add missing props. Created Base TagResourceStatistics class as well.
@trmcnvn
Copy link
Owner

trmcnvn commented Aug 10, 2019

I just noticed some of these issues myself when rewriting the README. Great timing! 👍

@trmcnvn trmcnvn self-requested a review August 10, 2019 08:24
@trmcnvn
Copy link
Owner

trmcnvn commented Aug 10, 2019

If we're going to have Update<Record> should we also rename the base ones to Create<Record> for clarity sake?

@trmcnvn trmcnvn mentioned this pull request Aug 10, 2019
Copy link
Owner

@trmcnvn trmcnvn left a comment

Choose a reason for hiding this comment

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

This is awesome work so far 🎉

DigitalOcean.API/Clients/LoadBalancerClient.cs Outdated Show resolved Hide resolved
DigitalOcean.API/Models/Requests/LoadBalancer.cs Outdated Show resolved Hide resolved
@nicholi
Copy link
Contributor Author

nicholi commented Aug 10, 2019

If we're going to have Update<Record> should we also rename the base ones to Create<Record> for clarity sake?

Since you raised it, I think so for sure. I'll add it now. I wasn't too sure at first about creating a duplicate class with just the class name being different, but it makes for a clean implementation and easy for future changes.

There was another case where one class was used as both Request/Response, StickySessions, used in LoadBalancer request and response. Should we split this one as well?

@trmcnvn
Copy link
Owner

trmcnvn commented Aug 10, 2019

If we're going to have Update<Record> should we also rename the base ones to Create<Record> for clarity sake?

Since you raised it, I think so for sure. I'll add it now. I wasn't too sure at first about creating a duplicate class with just the class name being different, but it makes for a clean implementation and easy for future changes.

There was another case where one class was used as both Request/Response, StickySessions, used in LoadBalancer request and response. Should we split this one as well?

Yeah, I just mentioned that in my first review.

@nicholi
Copy link
Contributor Author

nicholi commented Aug 10, 2019

I also found a some new (or missing) properties for certain request/response models, as well as few missing methods.

I'll be adding those as a second PR after this. I made this PR exclusively for all the things that would be clear breakages in API.

@trmcnvn trmcnvn mentioned this pull request Aug 10, 2019
20 tasks
@trmcnvn
Copy link
Owner

trmcnvn commented Aug 10, 2019

I also found a some new (or missing) properties for certain request/response models, as well as few missing methods.

I'll be adding those as a second PR after this. I made this PR exclusively for all the things that would be clear breakages in API.

I just made an issue noting the major missing features #45. Could you comment there if you work on any of those in your mentioned PR.

@nicholi
Copy link
Contributor Author

nicholi commented Aug 10, 2019

Going on a break for now. I think that covers everything mentioned so far.

@trmcnvn
Copy link
Owner

trmcnvn commented Aug 10, 2019

I noticed in Models/Requests/LoadBalancerDroplets.cs that the field is defined as int[] rather than List<int>.

Also.

public class Network {
        public List<Interface> v4 { get; set; }
        public List<Interface> v6 { get; set; }
    }

These should be V4, V6.

@nicholi
Copy link
Contributor Author

nicholi commented Aug 10, 2019

I noticed in Models/Requests/LoadBalancerDroplets.cs that the field is defined as int[] rather than List<int>.

Also.

public class Network {
        public List<Interface> v4 { get; set; }
        public List<Interface> v6 { get; set; }
    }

These should be V4, V6.

Done on both.

Something else I noticed about the Network.Interface response data. For ipv6 interfaces the netmask is always represented as an int instead of a string. The smart deserialization sorta just pushes it through as a string no matter what, so its not really an issue. However, do you think there should be an InterfaceV4 and InterfaceV6 class? That would be the only difference between them.

This ones a little odd because there doesn't seem to be any specific documentation on the Network object of the response. This is just what I've noticed from fully inspecting the responses.

@trmcnvn
Copy link
Owner

trmcnvn commented Aug 10, 2019

Yeah, I think having separate Interfaces is the correct move. They may differ further in the future.

@trmcnvn
Copy link
Owner

trmcnvn commented Aug 10, 2019

Could you also split Requests.Action into DropletAction and ImageAction? It's a bit odd that this one stands out as a one-size-fits-all model now.

@trmcnvn
Copy link
Owner

trmcnvn commented Aug 10, 2019

Could you rebase on to master when you're done so I can merge.

@trmcnvn trmcnvn merged commit 23c279e into trmcnvn:master Aug 10, 2019
trmcnvn pushed a commit that referenced this pull request Aug 10, 2019
* Switch to same enum style list call as Images

* Should have been imageIdOrSlug all along. imageId being an int and slug being a string.

* Rename ForwardingRules->ForwardingRule.
Add ForwardingRulesList, the list request used in adding/removing.
Explicit ForwardRule request (1 optional value type) and response class.
LoadBalancer request modified, 2 optional value types.

* Match filename to class name

* Rename Address to IpAddress (same as Networks and DomainRecords)
Forgot to removed un-necessary JsonPropertAttributes from response

* Create explicit HealthCheck request class (4 optional value type), remove un-necessary JsonPropertyAttributes from response class.
Rename type->Type

* Change currency properties from float to Decimal type

* Create Explicit UpdateImage class, convert Image to actual class for Create method.
Move ImageType enum to its own file.
Simplify GetAll switch logic.

* Explicit UpdateKey class (can only update name currently).
There is no UpdateFingerprint to Test.

* Tags are always strings.
4 optional value type properties.

* Rename request TagResource->TagResources, strongly typed request class for Tag/Untag methods.
Renamed request Resource->TagResource and move into its own file.
Rename Resource->TagResourcesStatistics, strongly typed respone class. Add missing props. Created Base TagResourceStatistics class as well.

* Rename to ForwardingRules, the methods operate on multiple items.

* Explicit UpdateDomainRecord class.

* Explicit Requests class for StickySessions.

* LoadBalancer id's are strings not ints

* DomainRecord doc updates

* Forgot [Fact]

* Consistent usage of List over array.

* Capitalization

* Doc update

* Should be a floating point, defaulting to double.

* Separate V4 and V6 Interface classes.
V6 uses integer for Netmask.

* Split Action request into DropletAction and ImageAction.
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.

2 participants