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

Fleshing out a few things. #62

Merged
merged 5 commits into from
Sep 2, 2020
Merged

Conversation

gregarican
Copy link
Contributor

Added a few API endpoint properties that were missing. A few DueDate fields changed to string data types, since the JSON response body wasn't properly parsing them as DateTime? elements. Added logic to the CustomFieldConverter class for instances where there are no custom fields defined.

… are exposed and accessible. The Activity DueDate didn't seem to parse as expected from the JSON response, so changed the data type to string. The CustomFieldConverter likely needs to be changed back on my commit to reference the new LongCustomField class, rather than the previous IntCustomField class.
Copy link
Owner

@DavidRouyer DavidRouyer left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@@ -0,0 +1,298 @@
using System;
Copy link
Owner

Choose a reason for hiding this comment

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

What is that file for? Please remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do,

@@ -0,0 +1,47 @@
using System;
Copy link
Owner

Choose a reason for hiding this comment

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

What is that file for? Please remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@@ -0,0 +1,59 @@
using System;
Copy link
Owner

Choose a reason for hiding this comment

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

What is that file for? Please remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@@ -0,0 +1,67 @@
using System;
Copy link
Owner

Choose a reason for hiding this comment

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

What is that file for? Please remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@@ -0,0 +1,132 @@
using System;
Copy link
Owner

Choose a reason for hiding this comment

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

What is that file for? Please remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@@ -27,7 +27,7 @@ public class Activity : IDealUpdateEntity
public string ReferenceId { get; set; }

[JsonProperty("due_date")]
public DateTime? DueDate { get; set; }
public string DueDate { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

Open a specific issue for that. I'll take a look, don't change that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Should I at least place a comment block here referencing the possible issue?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes you can!

@@ -27,7 +27,7 @@ public class DealActivity
public string ReferenceId { get; set; }

[JsonProperty("due_date")]
public DateTime? DueDate { get; set; }
public string DueDate { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

Open a specific issue for that. I'll take a look, don't change that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Should I at least place a comment block here referencing the possible issue?

[JsonProperty("done")]
public ActivityDone Done { get; set; }

[JsonProperty("type")]
public string Type { get; set; }

[JsonProperty("due_date")]
[JsonConverter(typeof(DateWithoutTimeConverter))]
public DateTime? DueDate { get; set; }
public string DueDate { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

Open a specific issue for that. I'll take a look, don't change that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Should I at least place a comment block here referencing the possible issue?

@@ -0,0 +1,13 @@
namespace Pipedrive.CustomFields
Copy link
Owner

Choose a reason for hiding this comment

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

This type has been replaced with LongCustomField, no need to recreate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will remove this file since it's no longer needed.

writer.WritePropertyName(field.Key);
writer.WriteValue(s.Value);
break;
case IntCustomField i:
Copy link
Owner

Choose a reason for hiding this comment

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

Don't override my work. I've replaced IntCustomField with LongCustomField in 2c50bfd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My apologies, as I forgot to include that update in this particular file.

@gregarican
Copy link
Contributor Author

I have made the necessary changes that were requested. Should I close this pull request and create a new one with my updated code?

@DavidRouyer
Copy link
Owner

You can update this one or create a new one!

@gregarican
Copy link
Contributor Author

The latest commit from my fork should contain the requested changes. Hope this helps!

@DavidRouyer
Copy link
Owner

Please remove the .bak files and undo the changes made to DueDate everywhere

@gregarican
Copy link
Contributor Author

Will do. I was using v0.1.10.0 so perhaps that was why the DueDate handling was different.

@DavidRouyer
Copy link
Owner

If you have a fork, sync it with my master branch and make changes from there, it will be simpler

@gregarican
Copy link
Contributor Author

Hopefully this will be the last commit. Sorry for the confusion. Your last set of requested changes have been made. If this suits what's needed then we are good to go!

@DavidRouyer DavidRouyer merged commit 210aced into DavidRouyer:master Sep 2, 2020
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