Skip to content
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

C#: Public method should be started with a capital letter #1664

Closed
cookgreen opened this issue Aug 10, 2020 · 10 comments
Closed

C#: Public method should be started with a capital letter #1664

cookgreen opened this issue Aug 10, 2020 · 10 comments
Labels
priority: low SWIG wontfix the fix would make the software much more complex and much less maintainable

Comments

@cookgreen
Copy link

cookgreen commented Aug 10, 2020

The methods generated by SWIG should be started with a capital letter

and many things should use Propety to do it rather than use method (For example, getSingleton() should be Singleton(property) or Instance(property))

Can you reference the Mogre project?

@paroj
Copy link
Member

paroj commented Aug 10, 2020

Mogre used a custom wrapper generator, which was tailored to Ogre and thus allowed to generate idiomatic C# code.

On the other hand, we now use SWIG which also handles Python and Java for us and is Ogre agnostic. Therefore the generated bings read just like C++ - this is also true for Python and Java BTW.

This does not mean, that the situation cannot be improved. I am sure one can achieve most of these things with SWIG as well. However, I do not use C# at all right now, so for one I dont know how idiomatic code should look like and this is not a priority for me.

Feel free to dig into the SWIG docs and open a PR: http://www.swig.org/Doc3.0/SWIGDocumentation.html

@paroj paroj added the SWIG label Aug 10, 2020
@paroj paroj changed the title [SWIG-C#] Public method should be started with a capital letter C#: Public method should be started with a capital letter Aug 10, 2020
@paroj
Copy link
Member

paroj commented Aug 15, 2020

@paroj paroj added this to the 1.13 milestone Aug 15, 2020
@cookgreen
Copy link
Author

http://www.swig.org/Doc3.0/SWIGDocumentation.html#SWIG_limiting_renaming

alright, can you guys upgreade the Mogre Wrapper?

@paroj
Copy link
Member

paroj commented Aug 17, 2020

on the other hand, the Microsoft Guidlines advise against capitalizaton:
https://docs.microsoft.com/en-gb/dotnet/standard/native-interop/best-practices

properites have to be declared manually using

%attribute(A, int, a, get_a, set_a);

https://github.com/swig/swig/blob/master/Lib/typemaps/attribute.swg

but would work in other languages like Python too.

@paroj
Copy link
Member

paroj commented Aug 17, 2020

alright, can you guys upgreade the Mogre Wrapper?

no. we use SWIG now.

@OGRECave OGRECave deleted a comment from cookgreen Aug 17, 2020
@paroj
Copy link
Member

paroj commented Aug 18, 2020

@AndreKamplingORGADATA as a active user of the C# bindings, what is your view on "Capitalization of Public Methods"?

On the one hand, it would make code more C# like and more compatible with old MOGRE code - on the other hand it will break your code and violates the MS interop guidelines.

@cookgreen
Copy link
Author

cookgreen commented Aug 18, 2020

@paroj Another thing, getSingleton() method should be Singleton property

@AndreKamplingORGADATA
Copy link
Contributor

In our company we follow the Naming Guidelines from Microsoft when it comes to C#.
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/naming-guidelines
For interoperability we don't have a guide we follow yet. We have to establish that.

From my personal opinion I like the "Native interoperability best practices" @paroj have posted.
Yes it breaks the C# language best practices.

But if you see code with other naming conventions it's immediately clear that it's interop code.
For us it's very easy to see if some Ogre C++ method is called, or if it's just a method from C#.

Breaking standards is not a nice thing, but I think the pros outweight the cons.
If you get one step further, one can argue that the "Native interoperability best practices" guideline is a standard by itself. If you're saying that, you aren't breaking anything.

Another thing beside capitalization are properties. You have no properties in C++. But if argue, keep it as it is, you should leave a getter method a method and not convert it into a property.

Another view would be that the Swig wrapper hides the interoperability and therefore the regular naming guideline of the C# language would apply. But in my opinion the interoperability border begins where the "real" C# code begins, where we call the wrapper functions. A colleague of mine argues differently, by saying the interoperability border is where the language change.

In summary, in my opinion getSingleton() should be kept as it is. It should not start with a capital and not be property.

@cookgreen
Copy link
Author

cookgreen commented Aug 18, 2020

In summary, in my opinion getSingleton() should be kept as it is. It should not start with a capital and not be property.

I think the most important thing is that SWIG C# is C# code, not C++ code, it looks weird if it doesn't follow the C# guideline, and it is very difficult to switch the codes from Mogre to SWIG C#, if you guys don't change the codes by following the C# guideline.

Don't forget that Mogre use Singleton, and Axiom (another C# library which re-implement the ogre3d with C#, it is already outdated ) use Instance, getSingleton is getter method of Singleton , so it should be Singleton property rather than a method. and setXXX, getXX should be merged into property as well because it is absolutly getter and setter.

@AndreKamplingORGADATA
Copy link
Contributor

As I wrote, it's a question about where to draw the border. You will find people aguing pro and other against...

@paroj paroj added the wontfix the fix would make the software much more complex and much less maintainable label Apr 12, 2021
@paroj paroj removed this from the 1.13 milestone Apr 12, 2021
@paroj paroj closed this as completed Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low SWIG wontfix the fix would make the software much more complex and much less maintainable
Projects
None yet
Development

No branches or pull requests

3 participants