-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Remove "Runtime" from all namespaces. #1956
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
Conversation
@codemzs out of curiosity what is your plan w.r.t. namespace sorting and the like? Not do it, other? |
@TomFinley I plan to automate that using a script in a separate change but if you insist we do it in this change that is also fine by me but please let me know. |
No, I do not insist, just wanted to know plan is all, thanks. |
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.
Great, thanks @codemzs.
@@ -1,13 +1,15 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. |
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.
Interesting, good one, I wonder if we should have some mechanism to check for this sort of thing, right now it's purely manual.
I don't think there's a better way for this PR (hard to slice this PR in to parts), but w/ 923 files changed, the GitHub web GUI is pretty inadequate for the task. If anyone is tired of opening each "Load diff" for each file, you can put this your browser's JS console to open all: [...document.getElementsByTagName('button')].filter(a=>a.innerText.trim() === 'Load diff').forEach(a=>a.click()) One less tedious step :) |
@@ -296,7 +296,7 @@ private static string GetCharAsString(char value) | |||
if (generatedClasses.IsGenerated(fieldType.FullName)) | |||
return generatedClasses.GetApiName(fieldType, rootNameSpace) + "." + enumAsString; | |||
else | |||
return generatedClasses.GetApiName(fieldType, "Runtime") + "." + enumAsString; | |||
return generatedClasses.GetApiName(fieldType, "") + "." + enumAsString; |
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.
fyi -- so anyone else reading this doesn't go and check, GetApiName()
seems to handle ""
:
machinelearning/src/Microsoft.ML.Legacy/Runtime/Internal/Tools/GeneratedClasses.cs
Lines 27 to 38 in a8b844c
public string GetApiName(Type type, string rootNamespace) | |
{ | |
string apiName = ""; | |
if (!_typesSymbolTable.TryGetValue(type.FullName, out ApiClass apiClass)) | |
apiName = GenerateIntenalName(type, rootNamespace); | |
else | |
apiName = apiClass.NewName; | |
if (!string.IsNullOrEmpty(rootNamespace)&& apiName.StartsWith(rootNamespace)) | |
return apiName.Substring(rootNamespace.Length + 1); | |
else return apiName; | |
} |
|
||
[assembly: EntryPointModule(typeof(TimeSeriesProcessing))] | ||
[assembly: EntryPointModule(typeof(TimeSeriesProcessingEntryPoints))] |
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.
@ganik, @montebhoover: I assume NimbusML isn't taking in the TimesSeries work yet. If so, the renaming of the EntryPoint might affect NimbusML.
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.
LGTM -- can you put in an issue to also fix up example code to remove Runtime
?
In the future, we may want to wrap a unit test around a duplicate of all of our example code to ensure it stays in sync.
@justinormont Good idea, will do that. Thanks for reviewing! |
Partially fixes #1697 by removing "Runtime" from all namespaces and that is it, no moving classes to other namespaces.