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

Few Codebase improvements #928

Closed
gethari opened this issue Sep 3, 2019 · 6 comments
Closed

Few Codebase improvements #928

gethari opened this issue Sep 3, 2019 · 6 comments
Labels
difficulty: unknown or n/a fix is unknown in difficulty status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap

Comments

@gethari
Copy link

gethari commented Sep 3, 2019

Issue Summary

I have ran through the repo, and found some of my findings here. If my findings are valid i will go ahead to send a PR.

Code style

The Qualifier this is redundant and there are 61 occurrences. Some people consider this as different. But ill leave this according to you as this is just a visual overhaul.

  • All the models must fall under SendGrid.Helpers.Mail.Models instead of SendGrid.Helpers.Mail

Unused methods and properties

In the ISendGridClient interface properties like UrlPath,Version,MediaType,AddAuthorization and the methods like DeserializeResponseBodyAsync and DeserializeResponseBodyAsync

Test project

The test project is referencing .NET CORE 1.0 . We should migrate it to the latest version of .NET Core atleast to 2.2.

And the Integration.cs is crossing 6K lines now, we should partition the file and split it so it would be easier to move forward

@thinkingserious thinkingserious added difficulty: unknown or n/a fix is unknown in difficulty status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap labels Sep 6, 2019
@thinkingserious
Copy link
Contributor

Hello @gethari,

Thanks for contributing to our C# community! We certainly welcome PRs and in this case a sample PR would be a great start to get the conversation started.

With best regards,

Elmer

@gethari
Copy link
Author

gethari commented Sep 12, 2019

@thinkingserious Please ignore the test project changes, i was experimenting with it. The other changes are

  • Removed this everywhere
  • Renamed namespace under proper titles
  • Renamed a few params as suggested by Resharper

@gethari
Copy link
Author

gethari commented Nov 20, 2019

@thinkingserious any update on this ?

@jnyrup
Copy link
Contributor

jnyrup commented Dec 12, 2019

@gethari Are you aware of the consequences to your suggestions?

  • Removing a member from an interface is a breaking change.
ISendGridClient myClient = ...
var urlPath = myClient.UrlPath; 
// No longer compile as the property is removed from the interface
  • Changing the namespace a type is a breaking change
SendGrid.Helpers.Mail.Attachment attachment = new Attachment(); 
// No longer compiles as the namespace has changed

@thinkingserious
Copy link
Contributor

Hello @gethari,

I'm closing this issue based on the referenced PR being closed as a duplicate of #981.

If you would like to contribute a separate PR with only the codebase improvements, we would greatly appreciate it.

With best regards,

Elmer

@thinkingserious
Copy link
Contributor

@jnyrup,

Thanks for taking the time to review the changes and provide feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: unknown or n/a fix is unknown in difficulty status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

No branches or pull requests

3 participants