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

Project Properties Updated #10

Closed

Conversation

BRyeGmoney
Copy link
Contributor

@BRyeGmoney BRyeGmoney commented Apr 27, 2017

-added address fields present in v1.8.0 of the PlanGrid API


This change is Reviewable

-added address fields present in v1.8.0 of the PlanGrid API
-was necessary for DateTime
public string Owner { get; set; }

[JsonProperty("start_date")]
public DateTime? StartDate { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

StartDate and EndDate should be Date?s for both classes as they're ISO dates and are nullable. Check out the project object to see their format.

Copy link

Choose a reason for hiding this comment

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

Ah, good point. So they never include a time component? In that case, yeah, @JuliusAlexanderIV is right and they should use our custom Date class instead. Also, I second my thanks! :)

@juliusiv
Copy link
Contributor

@BRyeGmoney looks good, thanks for updating this! I just have one comment.

-time not stored in start_date and end_date properties. ISO-8601 format
of YYYY-MM-DD. Using custom Date type
Copy link
Contributor

@juliusiv juliusiv left a comment

Choose a reason for hiding this comment

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

Looks good! Please squash everything into one commit and then go ahead and merge it. Thanks again for updating this @BRyeGmoney

@BRyeGmoney
Copy link
Contributor Author

@JuliusAlexanderIV @kswoll no problemo. i imagine you guys are doing the squashing as part of the merge?

@juliusiv
Copy link
Contributor

@BRyeGmoney thanks for the PR, I just merged it into the master branch so it's now available on Nuget!

@juliusiv juliusiv closed this Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants