Skip to content

feat: full refactor #67

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

Merged
merged 10 commits into from
Feb 28, 2022
Merged

feat: full refactor #67

merged 10 commits into from
Feb 28, 2022

Conversation

peterdeme
Copy link
Contributor

@peterdeme peterdeme commented Feb 18, 2022

Draft, I may add a few things next week.

  • Primary goal: Remove hacky json serialization and deserialozation logic
  • Modified the folder structure a little bit for standard .NET structure
  • Added async postfix to all async methods
  • Created interfaces for all clients. This will make it easier for our customers to unit test.
  • Moves model objects to Model namespace
  • Separated models from clients
  • Added StyleCop linter
  • Added documentation to all public methods and clients (spent a lot of time with this 😅)

Copy link
Member

@sierpinskid sierpinskid left a comment

Choose a reason for hiding this comment

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

mostly minor stuff

@peterdeme peterdeme force-pushed the modernization branch 2 times, most recently from e731377 to 118a6d8 Compare February 23, 2022 12:20
@peterdeme
Copy link
Contributor Author

@sierpinskid I agree, but this follows the convention of the other libraries, such as the javascript one.

@peterdeme peterdeme force-pushed the modernization branch 2 times, most recently from b3567be to 85f6c49 Compare February 23, 2022 13:48
/// <summary>
/// Partial update of an <see cref="Activity"/>.
/// </summary>
Task<ResponseBase> ActivityPartialUpdateAsync(string id = null, ForeignIdTime foreignIdTime = null, Dictionary<string, object> set = null, IEnumerable<string> unset = null);
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to add activity getters into client too.

It's a missing feature, later on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, how does that look like?

Copy link
Contributor

@ferhatelmas ferhatelmas left a comment

Choose a reason for hiding this comment

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

overall looks great, left a few small comments. Let's address them.

Also, good to review public method comments, didn't point all of them but there are wrong ones due to copy paste

Copy link
Contributor

@ferhatelmas ferhatelmas left a comment

Choose a reason for hiding this comment

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

LGTM

@peterdeme peterdeme merged commit f984dbd into master Feb 28, 2022
@peterdeme peterdeme deleted the modernization branch February 28, 2022 09:27
@github-actions github-actions bot mentioned this pull request Feb 28, 2022
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.

3 participants