-
Notifications
You must be signed in to change notification settings - Fork 8
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
chore: Make header names that always need to be sent constants #38
Conversation
Momento/HeaderInterceptor.cs
Outdated
|
||
namespace MomentoSdk | ||
{ | ||
class Header | ||
{ | ||
public readonly List<string> onceOnlyHeaders = new List<string>{"Agent"}; |
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.
Thank you for making this change.
Can you also make "Agent" and "Authorization" constants in this class and use them here and where you are actually setting the headers?
Momento/HeaderInterceptor.cs
Outdated
|
||
namespace MomentoSdk | ||
{ | ||
class Header | ||
{ | ||
private const string Agent = "Agent"; |
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.
I dont think we should add this in each file. Define once and reference in each class.
e.g. drop the constant private here
const string AGENT_KEY = "Agent";
const string AUTHORIZATION_KEY = "Authorization";
And then in DataGrpcManager reference in Headers.AGENT_KEY
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.
And just one note, you probably should looking up naming conventions for C# for constants AGENT_KEY
is what is done in Java. So, check the appropriate C# guideline. May be it is exactly how you have defined here.
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.
It seems like FullProductName
is its naming convention for constants.
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/const
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.
Thank you for doing this.
LGTM
This is a follow-up pr for the implementation option 2 mentioned here.