-
Notifications
You must be signed in to change notification settings - Fork 74
Nikithauc/typescript beta mainnamespace #288
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
src/GraphODataTemplateWriter/CodeHelpers/TypeScript/TypeScriptNamespace.cs
Outdated
Show resolved
Hide resolved
src/GraphODataTemplateWriter/CodeHelpers/TypeScript/TypeScriptNamespace.cs
Outdated
Show resolved
Hide resolved
| public string GetMainNamespace() | ||
| { | ||
| return TypeScriptMainNamespaceName; | ||
| return TypeScriptMainNamespaceNamePrefix + (ConfigurationService.Settings.Properties != null && ConfigurationService.Settings.Properties.ContainsKey("typescript.namespacePostfix") ? ConfigurationService.Settings.Properties["typescript.namespacePostfix"] : string.Empty); |
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.
style: Please consider splitting this into multiple lines. And if you use null-conditional operator:
| return TypeScriptMainNamespaceNamePrefix + (ConfigurationService.Settings.Properties != null && ConfigurationService.Settings.Properties.ContainsKey("typescript.namespacePostfix") ? ConfigurationService.Settings.Properties["typescript.namespacePostfix"] : string.Empty); | |
| var properties = ConfigurationService.Settings?.Properties; | |
| return properties?.ContainsKey("typescript.namespacePostfix") == true | |
| ? TypeScriptMainNamespaceNamePrefix + properties["typescript.namespacePostfix"] | |
| : TypeScriptMainNamespaceNamePrefix; |
| // Arrange | ||
| var languageStr = language.ToString(); | ||
| var directoryPostfix = isPhpBeta ? "PHPBeta" : languageStr; | ||
| var directoryPostfix = languageStr + (isBeta ? "Beta" : string.Empty); |
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.
not sure if case sensitivity matters here but for php would languageStr be "PHP" so the string ends up exactly the same. Since it's a passed in parameter i don't know that's guaranteed and if it matters.
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 think this is answered looking at how it's called from the PHPMultipleNamespaces file.
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.
This is fed from an enum that is local to the tests, so nobody will be able to change it by changing a string parameter somewhere. The test caller needs to specify an enum value.
Also this is only used as part of a directory name where case doesn't matter as long as you are on Windows :)
For beta version, the typescript typings top-level namespace is 'microsoftgraphbeta'. The existing code has main namespaces always set to 'microsoftgraph'. This PR brings in the following changes-