search

bunq / sdk_csharp

C# SDK for bunq API
MIT License
35 stars 22 forks source link

Rename constants to PascalCase #58

Open epels opened 6 years ago

epels commented 6 years ago

Steps to reproduce:

  1. Read the source code

What should happen:

  1. Constants are named in PascalCase

What happens:

  1. Constants are named IN_ALL_CAPS.

See also:

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions

Implementations

kid-cavaquinho commented 6 years ago

@epels @OGKevin did a quick look into 'const' keyword in the source and could not find any that was not in pascal case. Did I missed something or is this issue closed?

Thanks!

OGKevin commented 6 years ago

@antao I dont quite understand ?

Currently all constants are in CAPS, which is not correct according to the style guide in the OP. So the source code must be refactored to rename the constants in PascalCase as the style guide says!

kid-cavaquinho commented 6 years ago

@OGKevin my bad, you are right! 👍

OGKevin commented 6 years ago

To contribute you can follow this guide: https://guides.github.com/activities/forking/

The constants in https://github.com/bunq/sdk_csharp/tree/develop/BunqSdk/Model/Generated should not be changed as these must be changed on bunq’s end. As these files are generated.

Thanks for helping out 👍

On 27 Dec 2017, at 14:17, João Antão notifications@github.com wrote:

Hi again, @OGKevin https://github.com/ogkevin . I would like to start contributing to bunq oss for several reasons... I have a small contribution to start regarding this issue. But I cannot push the feature branch towards github. (I am getting a 403) Do I need special permissions?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bunq/sdk_csharp/issues/58#issuecomment-354112613, or mute the thread https://github.com/notifications/unsubscribe-auth/ARGTBpq1AacP0QVKGlgNOYYyPw7snem-ks5tEkOFgaJpZM4RJrfN.

kid-cavaquinho commented 6 years ago

Thanks for the input @OGKevin , closed the other PR's cause they did not followed the naming conventions. Hopefully in the next days I will look further into the codebase 👍

OGKevin commented 6 years ago

Due to this introducing a lot of breaking change, this will be moved to 0.13.0 release instead of 0.12.5`

OGKevin commented 6 years ago

@antao We can still finish up the PR tho!

OGKevin commented 6 years ago

@antao I think the constants in the non generated files are almost done 👏 🎊 👍.

The ones in the generated folder, ill take care of them once 0.13.0 is near as this requires change in the generator and will introduce breaking change. With this being said, lets keep this on hold for now and finish up the 0.12.5 milestone.