C# code generation does not observe C# naming conventions

[ThomasBarnekow]

C# uses Pascal case (e.g., PascalCase) for class, method, and property names, for example. C# uses camel case (e.g., camelCase) for variable and parameter names, among other things. The generated C# code uses an inconsistent naming scheme.

Firstly, properties use camel case. Examples include all properties in the Configuration class and the apiClient property in the generated XyzApi class. For example, the apiClient property should be called ApiClient to be in in line with the conventions.

Secondly, parameters in the generated operations are in PascalCase:

string TransformationPostFormData(string Upload, string DocTypeAndNumber, string DocTitle)

The correct form would be:

string TransformationPostFormData(string upload, string docTypeAndNumber, string docTitle)

This would also be in line with the swagger.json, in which all these parameters are in fact in camel case. The code generation seems to actively turn them into Pascal case.

Parameters of methods that are not generated based on the swagger.json are all in camel case. Thus, there is also some inconsistency.

[wing328]

Should be addressed in #927

[ThomasBarnekow]

@wing328 This is partially addressed. If you look at the other classes (e.g., ApiClient), you'll see the same issues.

[wing328]

@ThomasBarnekow please kindly take another look. I've also updated the indentation to use 4 spaces instead of 2.

[ThomasBarnekow]

@wing328 That looks better now. I've seen that variable names are now in camelCase. However, there are a few other issues we could fix:

• There's a bug in ApiClient.Deserialize. The comparison type.GetType() == typeof (Object) doesn't make a lot of sense, because type.GetType() always returns RuntimeType. This should read type == typeof (Object).
• Some comments are out of sync with the method signatures.
• The layout is not fully consistent (nitpick).

Do you want to fix that as well? I could propose some changes.

[wing328]

For 1, I've updated the PR with your suggested way of comparison.

Would you please elaborate on your 2nd and 3rd points with examples? I did a quick look again but couldn't find those you're referring to.

[ThomasBarnekow]

@wing328 Regarding number 2, here's an example:

        /// <summary>
        /// Deserialize the JSON string into a proper object
        /// </summary>
        /// <param name="content">HTTP body (e.g. string, JSON)</param>
        /// <param name="type">Object type</param>
        /// <returns>Object representation of the JSON string</returns>
        public object Deserialize(string content, Type type, IList<Parameter> headers=null) {
            ...
        }

        /// <summary>
        /// Get the API key with prefix
        /// </summary>
        /// <param name="obj">Object</param>
        /// <returns>API key with prefix</returns>
        public string GetApiKeyWithPrefix (string apiKeyIdentifier)
        {
            ...
        }

Note the headers parameter which does not have a corresponding param element. On the other hand, there's a type param element without a corresponding formal parameter. You see the same issue with the apiKeyIdentifier, for example.

In the following example, still pertaining to number 2, you turned parameter names into camelCase but did not change the comments:

        /// <summary>
        /// Update parameters based on authentication
        /// </summary>
        /// <param name="QueryParams">Query parameters</param>
        /// <param name="HeaderParams">Header parameters</param>
        /// <param name="AuthSettings">Authentication settings</param>
        public void UpdateParamsForAuth(Dictionary<String, String> queryParams, Dictionary<String, String> headerParams, string[] authSettings)

Regarding number 3, here's an example (note the curly brackets laid out differently):

        public string EscapeString(string str) {
            return str;
        }

        public FileParameter ParameterToFile(string name, Stream stream)
        {
            ...
        }

The first one is Java style. The second one is C# style. There is a mix of both. Again, that's a nitpick.

Are you using ReSharper or any other tool that helps you spot these and possibly layout the code in a consistent way?

[wing328]

@ThomasBarnekow thanks for the detailed feedback.

I'll look for tools to analyse the auto-generated C# code. Later may need help from you again to review one more time.

[ThomasBarnekow]

I use ReSharper. I have made changes to the ApiClient class that I could provide.

[ThomasBarnekow]

@wing328 This is off-topic, but you should also have a look at the overall architecture ...

For example, the ApiClient class is API-specific, because the constructor has a default parameter based on the swagger spec. Not sure whether that's a good idea. Everything else is unspecific. Further, default parameters should be avoided in APIs, because they will be compiled into the API client which would have to be recompiled if things (e.g., the URL in this case) change. But I'm not sure what the intention is.

Further, is it possible to change the namespace of those generated classes?

[wing328]

@ThomasBarnekow would be great if you can send me the diff of the ApiClient class.

Thanks for the feedback about the design. The default parameters were there before I started working on this project. I keep it as there may be valid use cases using the default parameters.

About customizing name space, you can do so by providing a config file (e.g. config.json) via the -c switch. Here is a list of valid JSON key for C# CLI:

$ java -jar modules/swagger-codegen-cli/target/swagger-codegen-cli.jar config-help -l csharp

CONFIG OPTIONS
    packageName
        C# package name (convention: Camel.Case), default: IO.Swagger

    packageVersion
        C# package version, default: 1.0.0

[ThomasBarnekow]

@wing328 Where do you want me to send that? I could also just send the original file.

[wing328]

Please send to my personal email address listed in my github profile.

[wing328]

@ThomasBarnekow pushed another update to the PR with better C# style based on your feedback.

[ThomasBarnekow]

@wing328 Thanks. Had a look. Here are some points ...

Starting with TransformationApi (generated from swagger.json), I would use a property BasePath (unless you can still change the name, because it's not really a "path", right?) rather than the following two methods. Further, GetBasePath(string) doesn't make a lot of sense. And, by the way, I've never seen that <value> comment.

        /// <summary>
        /// Sets the base path of the API client.
        /// </summary>
        /// <value>The base path</value>
        public void SetBasePath(String basePath)
        {
            this.ApiClient.BasePath = basePath;
        }

        /// <summary>
        /// Gets the base path of the API client.
        /// </summary>
        /// <value>The base path</value>
        public String GetBasePath(String basePath)
        {
            return this.ApiClient.BasePath;
        }

Moving on to the ApiClient class, it seems you didn't use my changes to the Deserialize method body. Apart from streamlining and cosmetic changes, I also fixed a bug. For example, if you don' pass a headers parameter when passing a type == typeof (Stream) the method will throw a NullReferenceException (see Match match = regex.Match(headers.ToString())).

[wing328]

@ThomasBarnekow thanks for the suggestion. The reason why we put basePath in ApiClient is that the same configuration can be shared by more than one Api class (e.g. PetApi, StoreApi, UserApi) so that developers can avoid setting the basePath manually for each Api Class.

For , please find more information from the below:
https://msdn.microsoft.com/en-us/library/azda5z79.aspx
http://stackoverflow.com/questions/15901716/the-purpose-of-using-both-value-and-summary-tags-in-visual-studio-xml-docume

For Deserialize, I've fixed the issue (the diff on files you sent me is not easy to spot changes so thanks again for your time to review)

[wing328]

FYI. Updated the PR to use fully-qualified name "System.Threading.Tasks.Task" as suggested by another user. #956

[wing328]

@ThomasBarnekow is there anything else you would like to change/fix before we merge the PR (#927) ?

[ThomasBarnekow]

@wing328 Nothing that I can see at the moment.

[wing328]

@ThomasBarnekow the PR has been merged into develop_2.0 branch.