Constants following PascalCase convention #71

kid-cavaquinho · 2017-12-27T16:45:43Z

This PR closes/fixes the following issues:

Closes Rename constants to PascalCase #58 (And what a struggle with that commit regex 🙃)
- Tested

OGKevin · 2017-12-27T17:03:26Z

@AnTao 😁 Nice on getting the commits right!

And thanks for this pull request!

OGKevin

A few questions and a note regarding environment constants.

OGKevin · 2017-12-27T21:32:04Z

BunqSdk.Tests/Context/ApiContextTest.cs


-        private static ApiContext apiContext;
+        private static ApiContext _apiContext;


Why the leading _ ?

I believe it was resharper doing some work 'automatically'. Shall I revert the underscore on all the private variables?

Yes please revert the _, unless there is a style guide entry that is supporting this _ 👍

OGKevin · 2017-12-27T21:32:04Z

BunqSdk.Tests/Model/Generated/Endpoint/AvatarTest.cs

@@ -58,8 +58,8 @@ private static string UploadAvatarAndGetUuid(byte[] fileContentByte)
        {
            var customHeaders = new Dictionary<string, string>
            {
-                {ApiClient.HEADER_ATTACHMENT_DESCRIPTION, ATTACHMENT_DECSRIPTION},
-                {ApiClient.HEADER_CONTENT_TYPE, CONTEN_TYPE},
+                {ApiClient.HeaderAttachmentDescription, ATTACHMENT_DECSRIPTION},


ATTACHMENT_DECSRIPTION is still in caps.

I might have missed this ones, changing it.

OGKevin · 2017-12-27T21:32:04Z

BunqSdk.Tests/Model/Generated/Endpoint/AvatarTest.cs

-                {ApiClient.HEADER_ATTACHMENT_DESCRIPTION, ATTACHMENT_DECSRIPTION},
-                {ApiClient.HEADER_CONTENT_TYPE, CONTEN_TYPE},
+                {ApiClient.HeaderAttachmentDescription, ATTACHMENT_DECSRIPTION},
+                {ApiClient.HeaderContentType, CONTEN_TYPE},


CONTEN_TYPE is still in caps.

There seems to be a few constants in this file that have not been refactored, please refactor them.

OGKevin · 2017-12-27T21:32:05Z

BunqSdk/Http/ApiClient.cs


-        private readonly HttpClient client;
+        private readonly HttpClient _client;


Why the leading _ ?

OGKevin · 2017-12-27T21:32:05Z

BunqSdk/Http/ApiClient.cs


-        private readonly ApiContext apiContext;
+        private readonly ApiContext _apiContext;


Why the leading _?

OGKevin · 2017-12-27T21:32:05Z

BunqSdk/Json/BunqJsonConvert.cs


-        private static bool isInitialized;
+        private static bool _isInitialized;


Why the leading _?

OGKevin · 2017-12-27T21:32:05Z

BunqSdk/Model/Core/Installation.cs


        public SessionToken SessionToken { get; private set; }
-        private readonly Id id;
-        private readonly PublicKeyServer publicKeyServer;
+        private readonly Id _id;


Why the leading _ ?

OGKevin · 2017-12-27T21:32:05Z

BunqSdk/Model/Core/Installation.cs

-        private readonly Id id;
-        private readonly PublicKeyServer publicKeyServer;
+        private readonly Id _id;
+        private readonly PublicKeyServer _publicKeyServer;


Why the leading _ ?

OGKevin

It seems that I have been sloppy with the initial constants names!

Please rename the constants as I’ve pointed out.

OGKevin · 2017-12-28T08:26:42Z

BunqSdk.Tests/Model/Generated/Endpoint/PaymentTest.cs

-        private const string PAYMENT_AMOUNT_EUR = "0.01";
-        private const string PAYMENT_CURRENCY = "EUR";
-        private const string PAYMENT_DESCRIPTION = "C# test Payment";
+        private const string Amount = "0.01";


Please rename this back to PaymentAmountEur as Amount can also refer to the Amount object.

OGKevin · 2017-12-28T08:26:42Z

BunqSdk.Tests/Model/Generated/Endpoint/PaymentTest.cs

-        private const string PAYMENT_CURRENCY = "EUR";
-        private const string PAYMENT_DESCRIPTION = "C# test Payment";
+        private const string Amount = "0.01";
+        private const string Currency = "EUR";


Please rename this back to PaymentCurrecnyEur

OGKevin · 2017-12-28T08:26:42Z

BunqSdk.Tests/Model/Generated/Endpoint/PaymentTest.cs

-        private const string PAYMENT_DESCRIPTION = "C# test Payment";
+        private const string Amount = "0.01";
+        private const string Currency = "EUR";
+        private const string Description = "C# test Payment";


Please rename this back to PaymentDescription. This is our standard on how to name constants. There should be a group identifier and in this case it is Payment.

OGKevin · 2017-12-28T08:26:42Z

BunqSdk.Tests/Model/Generated/Endpoint/PaymentTest.cs

-                {Payment.FIELD_AMOUNT, new Amount(PAYMENT_AMOUNT_EUR, PAYMENT_CURRENCY)},
-                {Payment.FIELD_DESCRIPTION, PAYMENT_DESCRIPTION},
-                {Payment.FIELD_COUNTERPARTY_ALIAS, COUNTER_PARTY_SELF}
+                {Payment.FIELD_AMOUNT, new Amount(Amount, Currency)},


Don't forget to refactor the renames on these lines as well.

OGKevin · 2017-12-28T08:26:42Z

BunqSdk.Tests/Model/Generated/Endpoint/RequestInquiryTest.cs

-        private const string FIELD_PAYMENT_DESCRIPTION = "C# test Payment";
-        private const string FIELD_STATUS = "ACCEPTED";
-        private const int INDEX_FIRST = 0;
+        private const string Amount = "0.01";


Same as above, here I was kinda sloppy with the constants name 🙈, Please rename them as described in my comment above.

OGKevin · 2017-12-28T08:26:42Z

BunqSdk.Tests/Model/Generated/Endpoint/TabUsageSingleTest.cs

-        private const string TAB_ITEM_FIELD_DESCRIPTION = "Super expensive java tea";
-        private const string FIELD_STATUS_WAITING = "WAITING_FOR_PAYMENT";
+        private const string TabDescription = "Pay the tab for Java test please.";
+        private const string StatusOpen = "OPEN";


TabStatusOpen

@OGKevin it should be TabUsageSingleStatusOpen following the style you describe above regarding the group, right? naming is hard

@AnTao Naming is indeed hard! 😁 but you will get used to it!

Yes TabUsageSingleStatusOpen sounds good 👍

OGKevin · 2017-12-28T08:26:42Z

BunqSdk.Tests/Model/Generated/Endpoint/TabUsageSingleTest.cs

-        private const string FIELD_STATUS_WAITING = "WAITING_FOR_PAYMENT";
+        private const string TabDescription = "Pay the tab for Java test please.";
+        private const string StatusOpen = "OPEN";
+        private const string Amount = "42.00";


TabAmountEur

OGKevin · 2017-12-28T08:26:42Z

BunqSdk.Tests/Model/Generated/Endpoint/TabUsageSingleTest.cs

+        private const string TabDescription = "Pay the tab for Java test please.";
+        private const string StatusOpen = "OPEN";
+        private const string Amount = "42.00";
+        private const string Currency = "EUR";


TabCurrencyEur

OGKevin · 2017-12-28T08:26:43Z

BunqSdk.Tests/Model/Generated/Endpoint/TabUsageSingleTest.cs

+        private const string Amount = "42.00";
+        private const string Currency = "EUR";
+        private const string TabItemDescription = "Super expensive java tea";
+        private const string StatusWaiting = "WAITING_FOR_PAYMENT";


TabStatusWaiting

OGKevin · 2017-12-28T08:26:43Z

BunqSdk.Tests/Model/Generated/Endpoint/TabUsageSingleTest.cs

-                {TabUsageSingle.FIELD_STATUS, FIELD_STATUS_OPEN},
-                {TabUsageSingle.FIELD_AMOUNT_TOTAL, new Amount(AMOUNT_EUR, FIELD_CURRENCY)}
+                {TabUsageSingle.FIELD_DESCRIPTION, TabDescription},
+                {TabUsageSingle.FIELD_STATUS, StatusOpen},


Don't forget to refactor these lines after the constants renames.

#58)

OGKevin

Future proofing and some small consistency comments.

We're almost there! 👍 👏 @AnTao

OGKevin · 2017-12-28T09:11:07Z

BunqSdk.Tests/Model/Generated/Endpoint/MonetaryAccountBankTest.cs

+        private const string MonetaryAccountBankSubStatus = "REDEMPTION_VOLUNTARY";
+        private const string MonetaryAccountBankReason = "OTHER";
+        private const string MonetaryAccountBankReasonDescription = "Because this is a test";
+        private const string MonetaryAccountBankCurrency = "EUR";


Lets make this name future proof, MonetaryAccountBankCurrencyEur. If we start supporting other currencies then there will be less names to change 👍

OGKevin · 2017-12-28T09:11:36Z

BunqSdk.Tests/Model/Generated/Endpoint/PaymentChatTest.cs

-        private const string Description = "Payment From C# Test";
-        private const string Text = "test msg send from C# test";
+        private const string PaymentChatAmountEur = "0.01";
+        private const string PaymentChatCurrency = "EUR";


Please suffix the currency Eur

OGKevin · 2017-12-28T09:12:17Z

BunqSdk.Tests/Model/Generated/Endpoint/TabUsageSingleTest.cs

+        private const string TabUsageSingleDescription = "Pay the tab for Java test please.";
+        private const string TabUsageSingleStatusOpen = "OPEN";
+        private const string TabAmountEur = "42.00";
+        private const string TabCurrency = "EUR";


Please suffix Eur and consistency

OGKevin · 2017-12-28T09:13:02Z

BunqSdk.Tests/Model/Generated/Endpoint/TabUsageSingleTest.cs

-        private const string StatusWaiting = "WAITING_FOR_PAYMENT";
+        private const string TabUsageSingleDescription = "Pay the tab for Java test please.";
+        private const string TabUsageSingleStatusOpen = "OPEN";
+        private const string TabAmountEur = "42.00";


Please be consistent 😝 TabAmountEur -> TabUsageSingleAmountEur

The constant is also used along with the TabItemShop entity. Changing it for consistency!

OGKevin

One small test amount comment.

After that LGTM

OGKevin · 2017-12-28T09:59:39Z

BunqSdk.Tests/Model/Generated/Endpoint/TabUsageSingleTest.cs

-        private const string FIELD_STATUS_WAITING = "WAITING_FOR_PAYMENT";
+        private const string TabUsageSingleDescription = "Pay the tab for Java test please.";
+        private const string TabUsageSingleStatusOpen = "OPEN";
+        private const string TabUsageSingleAmountEur = "42.00";


😮 42.00 is way to much :P it will drain the test account pretty fast haha! Please lower this amount.

OGKevin

LGTM

OGKevin · 2017-12-28T10:25:41Z

@sandervdo you're up, please 👀

OGKevin · 2017-12-28T10:30:21Z

Total tests: 36. Passed: 36. Failed: 0. Skipped: 0.

All tests are passing. 👏.

Because this PR doenst need any extra tests, I'm marking the Tested task as done.

sandervdo

Thanks for all your work in this PR! I mostly have very small comments, some names are plural whereas our convention is to not use plural names at all (we use PaymentIds -> allPaymentId for instance). Some comments will require a second look of @OGKevin as he's the SDK hero. 😄

sandervdo · 2017-12-28T11:02:50Z

BunqSdk.Samples/PaymentListSample.cs

-        private const string MESSAGE_LATEST_PAGE_IDS = "Latest page IDs: ";
-        private const string MESSAGE_SECOND_LATEST_PAGE_IDS = "Second latest page IDs: ";
-        private const string MESSAGE_NO_PRIOR_PAYMENTS_FOUND = "No prior payments found!";
+        private const string MessageLatestPageIds = "Latest page IDs: ";


Plural -> MessageLatestPageAllPaymentId

sandervdo · 2017-12-28T11:03:14Z

BunqSdk.Samples/PaymentListSample.cs

-        private const string MESSAGE_SECOND_LATEST_PAGE_IDS = "Second latest page IDs: ";
-        private const string MESSAGE_NO_PRIOR_PAYMENTS_FOUND = "No prior payments found!";
+        private const string MessageLatestPageIds = "Latest page IDs: ";
+        private const string MessageSecondLatestPageIds = "Second latest page IDs: ";


Plural -> MessageSecondLatestPageAllPaymentId

sandervdo · 2017-12-28T11:03:34Z

BunqSdk.Samples/PaymentListSample.cs

-        private const string MESSAGE_NO_PRIOR_PAYMENTS_FOUND = "No prior payments found!";
+        private const string MessageLatestPageIds = "Latest page IDs: ";
+        private const string MessageSecondLatestPageIds = "Second latest page IDs: ";
+        private const string MessageNoPriorPaymentsFound = "No prior payments found!";


Plural -> MessageNoPriorPaymentFound

sandervdo · 2017-12-28T11:06:21Z

BunqSdk.Samples/PaymentListSample.cs

            };
-            Console.WriteLine(MESSAGE_LATEST_PAGE_IDS);
-            var paymentResponse = Payment.List(apiContext, USER_ITEM_ID, MONETARY_ACCOUNT_ITEM_ID,
+            Console.WriteLine(MessageLatestPageIds);


MessageLatestPageAllPaymentId

sandervdo · 2017-12-28T11:07:11Z

BunqSdk.Samples/PaymentListSample.cs

                paginationCountOnly.UrlParamsCountOnly);
            PrintPayments(paymentResponse.Value);
            var pagination = paymentResponse.Pagination;

            if (pagination.HasPreviousPage())
            {
-                Console.WriteLine(MESSAGE_SECOND_LATEST_PAGE_IDS);
-                var previousPaymentResponse = Payment.List(apiContext, USER_ITEM_ID, MONETARY_ACCOUNT_ITEM_ID,
+                Console.WriteLine(MessageSecondLatestPageIds);


Rename (check for other occurrences too of renamed constants)

sandervdo · 2017-12-28T11:24:03Z

BunqSdk.Tests/README.md

@@ -34,4 +34,6 @@ same directory, fill in your sandbox user data and rename the copy to `config.js

 ## Execution
 To run tests via Rider, you can right click on the `BunqSdk.Tests` solution and should be able to run
-the tests cases form the IDE.
+the tests cases from the IDE.
+To run the tests via Visual Studio, you can use the Test Explorer window. Use default shortcut Ctrl+R, A


Is there a shortcut for the Mac edition of Visual Studio too?

For VS Code there is an [extension] (https://github.com/formulahendry/vscode-dotnet-test-explorer) for testing. Although I use VS Code more and more frequently, I still do 95% of .NET development with Visual Studio, so I don't have much feedback on this extension, yet. I am also not sure if this shortcut will work with VS Code.

Are you guys using Rider? What is your feedback? (Sorry for the off topic of the PR)

Yea I use Rider, please list this extension in the README as well then to ensure that others know that they must install the extension 👍

@AnTao myself I don't code in C# at all, but we use the Jetbrains editors mostly yes (PhpStorm, PyCharm, etc.). But in my case, no Rider.

@OGKevin can you check for the shortcut on MacOS then?

sandervdo · 2017-12-28T11:25:23Z

BunqSdk/Context/ApiContext.cs


        /// <summary>
        /// Minimum time to session expiry not requiring session reset.
        /// </summary>
-        private const int TIME_TO_SESSION_EXPIRY_MINIMUM_SECONDS = 30;
+        private const int TimeToSessionExpiryMinimumSeconds = 30;


TimeToSessionExpiryMinimumSecond (hmm, looks weird, @OGKevin 👀 )

🤷‍♂️ your suggestion is fine, 🙅‍♂️ plurals.

@OGKevin @sandervdo TimeToSessionExpiryAllMinimumSecond 🙈 This renaming changes will bring a lot of breaking changes!

@AnTao How do you mean ? I only see one usage for it which is here:

sdk_csharp/BunqSdk/Context/ApiContext.cs

Line 196 in c71114b

TIME_TO_SESSION_EXPIRY_MINIMUM_SECONDS

Ow wait shit your right! I understand. Therefore this pull request needs to actually wait for 0.13.0 👍 Because this is indeed a breaking change! Holy crap how did I miss this 😅

@OGKevin actually the ones I changed in ApiContext.cs are all with the private modifier, so that will be alright.

@AnTao Correct, but the field constants for the generated code for instance all will be used publicly, means current implementations will break!

Lets continue the breaking change discussion in the issue please 👍

sandervdo · 2017-12-28T11:26:51Z

BunqSdk/Exception/ExceptionFactory.cs


        /// <summary>
        /// Glue to concatenate the error messages.
        /// </summary>
-        private const string GLUE_ERROR_MESSAGES = "\n";
+        private const string GlueErrorMessages = "\n";


ErrorMessageGlue

GlueAllErrorMessage Group by Glue

I don't like the Glue too much. Why don't we use Separator or sth?

Hmm well Separator can also be used just fine 👍

sandervdo · 2017-12-28T11:27:39Z

BunqSdk/Http/ApiClient.cs

+        private const string DeviceServerUrl = "device-server";
+        private const string InstallationUrl = "installation";
+        private const string SessionServerUrl = "session-server";
+        private static readonly string[] UrisNotRequiringActiveSession = new string[]


AllUriNotRequiringActiveSession

sandervdo · 2017-12-28T11:31:15Z

BunqSdk/Security/SecurityUtils.cs

-        private const string HEADER_NAME_PREFIX_X_BUNQ = "X-Bunq-";
-        private const string DELIMITER_HEADER_VALUE = ",";
-        private const string FORMAT_HEADER_STRING = "{0}: {1}";
+        private const string Newline = "\n";


@OGKevin do you want these constants grouped as well? Could use Format prefix.

Ah yeas Format prefix would indeed be great 👌.

…E. (#58)

OGKevin · 2017-12-28T20:55:51Z

@AnTao FYI I've pushed some minor changed, be sure to pull before you push new changes 👍

kid-cavaquinho · 2017-12-29T07:34:04Z

Thanks for the push @OGKevin

OGKevin · 2017-12-29T07:43:32Z

@AnTao I dont understand ? Why did you close this pull request ? There is nothing wrong with it!

kid-cavaquinho · 2017-12-29T07:51:51Z

@OGKevin I did not even noticed I've closed it. My bad! Thanks for re-opening it.

…58)

Joao Antao added 4 commits December 27, 2017 17:34

Rename constants following PascalCase convention. (#58)

d50b8b8

Add curly braces around the nested statement in this if block. (#58)

46214e0

Add information to test readme file on how to run tests in VS IDE. (#58)

8fd7136

Update gitignore file. (#58)

3fbb0e1

OGKevin self-requested a review December 27, 2017 17:01

OGKevin assigned OGKevin and kid-cavaquinho Dec 27, 2017

OGKevin added this to the 0.12.5 milestone Dec 27, 2017

OGKevin reviewed Dec 27, 2017

View reviewed changes

antao added 2 commits December 27, 2017 23:05

Remove underscore prefix from private variables. (#58)

9b16490

Rename constants following PascalCase convention. (#58)

17d446c

bunq deleted a comment Dec 27, 2017

OGKevin suggested changes Dec 28, 2017

View reviewed changes

Rename constants following group identifier and PascalCase convention. (

f405de3

#58)

bunq deleted a comment Dec 28, 2017

OGKevin suggested changes Dec 28, 2017

View reviewed changes

Rename constants consistently. (#58)

4a90c80

bunq deleted a comment Dec 28, 2017

OGKevin suggested changes Dec 28, 2017

View reviewed changes

Pink Floyds TDSOTM is 42 minutes lenght and expensive Java teas. (#58)

c43eb8c

bunq deleted a comment Dec 28, 2017

OGKevin previously approved these changes Dec 28, 2017

View reviewed changes

OGKevin assigned sandervdo Dec 28, 2017

OGKevin requested a review from sandervdo December 28, 2017 10:25

sandervdo suggested changes Dec 28, 2017

View reviewed changes

OGKevin modified the milestones: 0.12.5, 0.13.0 Dec 28, 2017

Joao Antao added 2 commits December 28, 2017 18:18

Add information to test readme file on how to run tests in VS Code ID…

d453229

…E. (#58)

Remove redundant string array. (#58)

2da16bf

kid-cavaquinho dismissed OGKevin’s stale review via 2da16bf December 28, 2017 17:32

bunq deleted a comment Dec 28, 2017

bunq deleted a comment from kid-cavaquinho Dec 28, 2017

OGKevin added 8 commits December 28, 2017 21:49

Reordered constants to their group. (#58)

d66b682

Added missing constant pre and suffixes. (#58)

2279dc8

Reordered constant to their separate group. (#58)

5f8e799

Added missing summaries. (#58)

c153018

Moved property with accessors below the ones without. (#58)

1aa819d

Removed plural. (#58)

9962a6d

Changed method scope to private. (#58)

f10e0d3

Removed plural from example test configuration josn. (#58)

50a81c3

bunq deleted a comment Dec 28, 2017

kid-cavaquinho closed this Dec 29, 2017

OGKevin reopened this Dec 29, 2017

Add information to test readme file on how to run tests in cmd line. (#…

4240c77

…58)

OGKevin modified the milestones: 0.13.0, 0.13.5 Mar 27, 2018

OGKevin modified the milestones: 1.0.0, 1.1.0 Jul 24, 2018

angelomelonas unassigned sandervdo Sep 16, 2020

angelomelonas removed this from the 1.1.0 milestone Sep 16, 2020


		private static ApiContext apiContext;
		private static ApiContext _apiContext;


		private readonly HttpClient client;
		private readonly HttpClient _client;


		private readonly ApiContext apiContext;
		private readonly ApiContext _apiContext;


		private static bool isInitialized;
		private static bool _isInitialized;

Constants following PascalCase convention #71

Are you sure you want to change the base?

Constants following PascalCase convention #71

Conversation

kid-cavaquinho commented Dec 27, 2017 • edited by OGKevin Loading

This PR closes/fixes the following issues:

OGKevin commented Dec 27, 2017

OGKevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin left a comment

Choose a reason for hiding this comment

OGKevin commented Dec 28, 2017

OGKevin commented Dec 28, 2017 • edited Loading

sandervdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin commented Dec 28, 2017

kid-cavaquinho commented Dec 29, 2017

OGKevin commented Dec 29, 2017

kid-cavaquinho commented Dec 29, 2017

kid-cavaquinho commented Dec 27, 2017 •

edited by OGKevin

Loading

OGKevin commented Dec 28, 2017 •

edited

Loading

OGKevin Dec 28, 2017 •

edited

Loading