refactor: Clarify naming of profiler constants (fix #602 and fix #603) - #611
Conversation
| new MLAPIProfilerCounter { m_Name = ProfilerConstants.RPCsSent, m_Category = ProfilerCategory.Network.Name }, | ||
| new MLAPIProfilerCounter { m_Name = ProfilerConstants.RPCsReceived, m_Category = ProfilerCategory.Network.Name }, | ||
| new MLAPIProfilerCounter { m_Name = ProfilerConstants.RPCBatchesSent, m_Category = ProfilerCategory.Network.Name }, | ||
| new MLAPIProfilerCounter { m_Name = ProfilerConstants.RPCBatchesReceived, m_Category = ProfilerCategory.Network.Name }, | ||
| new MLAPIProfilerCounter { m_Name = ProfilerConstants.RPCQueueProcessed, m_Category = ProfilerCategory.Network.Name }, | ||
| new MLAPIProfilerCounter { m_Name = ProfilerConstants.RPCsInQueueSize, m_Category = ProfilerCategory.Network.Name }, | ||
| new MLAPIProfilerCounter { m_Name = ProfilerConstants.RPCsOutQueueSize, m_Category = ProfilerCategory.Network.Name }, |
There was a problem hiding this comment.
RPC should be capitalized as Rpc not RPC according to naming standards.
But RpcsSent doesn't look great to me either — so, should we drop plurals since they are re numbers/counts?
I'm thinking RpcSent or NumRpcSent etc.
There was a problem hiding this comment.
also isn't this a refactor rather than a fix?
I think PR/commit title should be refactor: clarify naming... instead of fix: clarify naming...
There was a problem hiding this comment.
See, I didn't know about the refactor thing lol. Makes sense. Will correct title. Will also remove the S. But since it was for the profiler, it's understood it's a number, hence why we don't want Num or Number. I'll update the PR.
Rosme
changed the title
| private static List<MLAPIProfilerCounter> CreateOperationsCounters() => new List<MLAPIProfilerCounter>() | ||
| { | ||
| new MLAPIProfilerCounter { m_Name = ProfilerConstants.NumberOfConnections, m_Category = ProfilerCategory.Network.Name }, | ||
| new MLAPIProfilerCounter { m_Name = ProfilerConstants.Connection, m_Category = ProfilerCategory.Network.Name }, |
There was a problem hiding this comment.
I think this one should still be plural, although Kamau already made that change in #613 so may be worth pulling this one out to avoid conflicts
0xFA11
left a comment
0xFA11
left a comment
There was a problem hiding this comment.
apparently — you have some conflicts to resolve but apart from that 🚀
|
Conflict should be fixed and works now |
4 participants
No description provided.