-
Notifications
You must be signed in to change notification settings - Fork 18
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
Addressing issue 9 - partial fix for moving the models to separate class #10
base: master
Are you sure you want to change the base?
Conversation
dont add mentions if they exceed character limit
…it-fix length fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is OK. I'd prefer the folder to be named "Models" to be inline with regular .Net standards.
Also it is better to put a using
statement uptop instead of qualifying the model as thehexbot.models.Configuration
everywhere we need it.
Tweetinvi api has also one configuration.cs file so accessing thehexbot.Models.Configuration as config. In C# two classes having same name with different name space, we have to explicitly mention the full namespace and class name |
and also renamed models to Models |
@@ -8,7 +8,7 @@ | |||
using Tweetinvi; | |||
using Tweetinvi.Parameters; | |||
using Tweetinvi.Models; | |||
|
|||
using config = thehexbot.models; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this like? A simple using thehexbot.Models
should allow us to use Configuration
right? Why config.Configuration
? It seems redundant
Tweetinvi api has also one configuration.cs file so accessing thehexbot.Models.Configuration as config. In C# two classes having same name with different name space, we have to explicitly mention the full namespace and class name |
this pull request is part of addressing issue 9. I have moved the models to separate classes, if it agrees with your coding conventions, approve the pull request, also i will start moving the core logic to separate classes
Thanks
Balaji